search

SWI-Prolog / packages-cpp

The SWI-Prolog C++ interface
30 stars 14 forks source link

ENHANCED: C++-compatible exception handling + more wrapper functions and classes #39

Closed kamahen closed 1 year ago

kamahen commented 1 year ago

SWI-cpp2-plx.h contains wrapper functions (imported from SWI-cpp2.h)

======

This is a pretty big set of changes, but it would have been difficult to break it up into smaller pieces.

The documentation is still somewhat rough, but a lot of things can be easily understood from reading the code or the test cases.

JanWielemaker commented 1 year ago

This pull request has been mentioned on SWI-Prolog. There might be relevant details there:

https://swi-prolog.discourse.group/t/cpp2-exceptions/6040/77

kamahen commented 1 year ago

My apologies about the multiple commits -- something went wrong with either my emacs session or git (or both) and a section of pl2cpp2.doc was duplicated onto the end of the file. When this PR is accepted, please do a "squash".

If you wish, I can re-submit a new PR with a single commit.

kamahen commented 1 year ago

I've opened bugs https://github.com/SWI-Prolog/packages-cpp/issues/40 and https://github.com/SWI-Prolog/packages-cpp/issues/41 for some problems with PlRecord and PlQuery (and possibly one or two other classes). The current functionality is not worse than the original SWI-cpp.h functionality and it could take me a few weeks (or more) to make the required improvements, so I suggest merging this PR without fixing those two issues. But I don't have a strong opinion on that - just that this PR is already too big. (I could make a new commit with the fixes; but I'd like the reviews to look at the rest of the code first)

JanWielemaker commented 1 year ago

Using PL_record_external() is broken as it doesn't respect blobs. I propose to fix PlRecord first and use that for exceptions. That is still a far from elegant solution copying exceptions 4 times rather than once, but disregarding time and space requirements, it is at least correct.

kamahen commented 1 year ago

I originally used PL_record() in PlException but it also crashed, so I switched to PL_record_external(). I'll try PL_record again, but I probably won't be able to work on it for a little while, possibly more than a month. In the meantime, could the review of other things continue, please?

It would be nice to also fix PlRecord to properly do PL_duplicate_record() and PL_erase() in its constructors (the move constructor needs to be added) and destructor [or do its own reference counting], but I don't think that would be necessary for it to work with PlException.

(If I can't fix PlRecord, I'll document its current limitations and how I plan to fix them in a backwards compatible way)

kamahen commented 1 year ago

I tried PlRecord instead of PlRecordExternalCopy in PlExceptions -- it passed all my tests, but failed an ASAN test with rocksdb. I'll look into that, and if I can fix it, I'll switch to PlRecord. (Eventually, hope to get rid of PlRecord entirely and just use the exception term.)

The updated PlRecord is still "manual" -- that is, it doesn't use PL_duplicate_record() in the copy/move constructors ... that's a future enhancement.

kamahen commented 1 year ago

The latest commit changes from PlRecordExternalCopy to PlRecord and it passes all my tests. In future, I hope to change this to PlTerm, but not in this PR.

Also in the latest commit were some changes to the constructors (including both copy and move constructors) that seemed to be needed by some strange error messages from g++, and listing all the constructors and operator= with either default or delete made the messages go away. (I suspect the messages come from the deprecated operator bool(), but I'm not sure)

JanWielemaker commented 1 year ago

Thanks! Merged after rebase and fixing one typo for a Windows API function prototype.