Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 10 commits into from

Conversation

kamahen
Copy link
Contributor

@kamahen kamahen commented Apr 8, 2023

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

  • throw PlException for Prolog errors
  • most SWI-Prolog.h functions have a wrapper. verify() methods removed from PlAtom, PlTerm, etc.
  • the wrapper functions do the checking. Some executable code has been moved to SWI-cpp2.cpp
  • can be inlined, if desired. PlException is now a subclass of std::exception, not a subclass of PlTerm. PlTypeError, PlDomainError, etc. are no longer subclasses of PlTerm,
    but are functions for creating suitable PlException objects.
    The string comparison operators are deprecated; use as_string()
    and std::string comparison instead, which allows specifying the encoding.
    Added PlRecord, PlRecordExternal, PlControl (used by PREDICATE_NONDET), PlStream. Fixed numerous bugs and misfeatures; added tests.

======

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.

…and classes

SWI-cpp2-plx.h contains wrapper functions (imported from SWI-cpp2.h)
- throw PlException for Prolog errors
- most SWI-Prolog.h functions have a wrapper.
verify() methods removed from PlAtom, PlTerm, etc.
- the wrapper functions do the checking.
Some executable code has been moved to SWI-cpp2.cpp
- can be inlined, if desired.
PlException is now a subclass of std::exception, not a subclass of PlTerm.
PlTypeError, PlDomainError, etc. are no longer subclasses of PlTerm,
  but are functions for creating suitable PlException objects.
The string comparison operators are deprecated; use as_string()
  and std::string comparison instead, which allows specifying the encoding.
Added PlRecord, PlRecordExternal,  PlControl (used by PREDICATE_NONDET),  PlStream.
Fixed numerous bugs and misfeatures; added tests.
@JanWielemaker
Copy link
Member

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

…and classes

SWI-cpp2-plx.h contains wrapper functions (imported from SWI-cpp2.h)
- throw PlException for Prolog errors
- most SWI-Prolog.h functions have a wrapper.
verify() methods removed from PlAtom, PlTerm, etc.
- the wrapper functions do the checking.
Some executable code has been moved to SWI-cpp2.cpp
- can be inlined, if desired.
PlException is now a subclass of std::exception, not a subclass of PlTerm.
PlTypeError, PlDomainError, etc. are no longer subclasses of PlTerm,
  but are functions for creating suitable PlException objects.
The string comparison operators are deprecated; use as_string()
  and std::string comparison instead, which allows specifying the encoding.
Added PlRecord, PlRecordExternal,  PlControl (used by PREDICATE_NONDET),  PlStream.
Fixed numerous bugs and misfeatures; added tests.
@kamahen
Copy link
Contributor Author

kamahen commented Apr 10, 2023

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
Copy link
Contributor Author

kamahen commented Apr 13, 2023

I've opened bugs #40 and #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
Copy link
Member

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
Copy link
Contributor Author

kamahen commented Apr 14, 2023

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
Copy link
Contributor Author

kamahen commented Apr 14, 2023

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
Copy link
Contributor Author

kamahen commented Apr 16, 2023

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
Copy link
Member

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

@kamahen kamahen deleted the exceptions-cleaned branch April 19, 2023 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants