search

SWI-Prolog / packages-pcre

SWI-Prolog package for access to Perl Regular Expressions
5 stars 7 forks source link

ENHANCED: added save/load functionality to pcre blob #29

Closed kamahen closed 1 year ago

kamahen commented 1 year ago

Requires https://github.com/SWI-Prolog/swipl-devel/pull/1149

I don't like the "save_load" test -- I'd prefer something that can do a stand-alone compile of test_load_pcre.pl without loading it, but swipl -c seems to have no option for omitting the runtime. (The reason I dislike the test - besides its clunkiness - is that qcompile/1 adds to the namespace ... the test abolishes all the created predicates (I think), but it would be nicer if the namespace didn't get modified at all by the compilation step.)

JanWielemaker commented 1 year ago

Merged. Thanks. Mostly changed the test to reuse (after some improvement) the much simpler scheme of the qlf test in the core. Compiling .qlf files must load the code as well as (in SWI-Prolog) we allow the compilation step to use code defined earlier in the same file. There are good reasons to allow for this as well as not to allow for this and multiple Prolog systems in both camps ... The improved version cleans the qlf file and uses unload_file/1 to cleanup from the original.

That guarantees the qlf file is actually loaded. Of course, symbols may be left from the compilation. The reload should work while the loaded symbols pre-exist and not. You can ensure no such symbol exists by using a new Prolog process for compiling the Qlf file using -g command -t halt. You may also call the garbage_collect_atom/0 after the reload. As atom-GC is conservative, there is no guarantee.

kamahen commented 1 year ago

I did a quick test of load failure by adding a || 1 to the if (!load...) ad pcre4pl.c:326, so that the error path is always taken, and it produced this output -- is it what you expected?

?- run_tests(pcre:save_load).
[1/1] pcre:save_load ..
ERROR: Failed to load Pcre2 blob
   Call: (84) close(<stream>(0x5579ec962b00)) ? l
ERROR: /home/peter/src/swipl-devel/packages/pcre/test_pcre.pl:836:
ERROR:     test save_load: assertion at /home/peter/src/swipl-devel/packages/pcre/input/pcre_load.pl:48 failed
    Assertion: blob(_1026,regex)
% No tests to run
false.

[debug]  ?-
JanWielemaker commented 1 year ago

Not really. There is no error recovery from qlf load issues. Most of it calls fatalError(). getBlob() must return a valid atom or you will end up with some invalid atom in a place where a valid atom is expected and thus (most likely) crash sooner or later. I guess a load function can, if it can guarantee that it read everything that was written by its save function (and nothing more) return an atom that indicates an error status. Still, I can't see a reasonable scenario for that.

I guess we could do a lot more checking and stop loading a .qlf file on error. For a saved state that makes no sense: if you can't load it you can't continue. For a .qlf file you could continue, but you remain in a state where some code has been added and directives have been executed leaving unknown changes to the system, so there is little hope for a safe continuation.