@Jan sent me a private note with some suggestions and questions. I’ll paraphrase his comments here. (And a few I’ll put as “code review” comments)
All classes in the end embed (only) a C handle. Doesn’t it make sense
to have the same method for accessing this, e.g. c, so we can do
PlFunctor f;
...
PL_something(f.c(), ...)
I know it is common to provide access functions for members to keep
binary compatibility, but as this is only inline code I doubt that makes
much sense, so f.c and c as a public member would do as well?
The general convention with C++ is for struct to be for POD (plain old data) that might have some convenience functions and class is for everything else; the biggest difference is whether the default visibility is “public” (struct) or “private” (class); and typically fields in a class follow a convention (suffix “_” at Google, prefix “m_” at GNU_, etc.). The C++ wrappers in SWI-cpp.h seem to be half-way in between – they’re POD in the sense that they just wrap a handle, but they provide quite a few “convenience” functions, so I’ve decided that they should be “class”.
As Jan says, there’s no need for getters/setters for binary compatibility; the question is how to access the underlying handle when needed. The compiler will expand PL_something(f.c(), ...) to PL_something(f.functor_, ...) so efficiency isn’t the issue. In general, when someone wants to access a PL_something() in SWI-Prolog.h, we should want to make that available in SWI-cpp.h (as f.something(...)), so the convenience of accessing the wrapped handle shouldn’t be a significant issue.
Perhaps leave the handle private and access it by C()? Making the method name upper-case would also make it easy to find with a regular text editor.
Why PlAtom::reset() and PlTerm::reset()?
I’m following the style of “smart pointers”, by having a function for setting the value to “null” (for atom_t and term_t, the “null” is 0, although I don’t think it’s documented anywhere). I forgot to include PlAtom::reset(atom_t) and PlTerm::reset(term_t). [I’ve also created “null” constants – they’re “private” for now.].
If we access the wrapped handle using C(), then we need a method for setting the value. If we make the wrapped handle public (as in the original SWI-cpp.h, then there’s no need for C() and reset(). I don’t have a strong opinion on this; the atom and term wrappers aren’t really smart pointers, so it could be confusing to follow the smart pointer conventions.
(We could, of course define a dereference operator (*) instead of C(), but I think that would be confusing because atom_t and term_t don’t always act like pointers in the SWI-Prolog.h functions.
some questions about is_valid()
The idea is that a constructor should be written like this:
PlAtom::PlAtom(const char *text)
: handle_(PL_new_atom(text)) { ... }
and not like this:
PlAtom::PlAtom(...) {
handle_ = PL_new_atom(text);
....
}
If the system guarantees that PL_new_atom() always succeeds or else terminates the program with a fatal error, then there’s no need for is_valid() in the constructor.
If PL_new_atom() fails due to out-of-memory, does that always give a fatal error? From looking at various bits of foreign function code and swipl internal code, it appears that a “resource error” is a possibility; and that’s why I added the test to the constructors.
However, there’s a second use for is_valid() – some code uses a “null” value (that is: 0) to indicate that the value hasn’t been set. We could create a operator bool() to check for validity, but there is also a “get_bool” defined for terms (which calls PL_get_bool(), which checks whether the term is an atom with the value true or false (and throws an exception if it’s any other value). This is confusing, so I decided to remove the operator bool() and require the check to be explicit.
[BTW, Jan originally wrote PlAtom constructor an initializer list, but some other constructors weren’t that way and I’ve modified them to use an initializer list where possible]
I’m unsure about the unify_*_ex() functions. Seems a lot of stuff that is barely ever needed. Why not PlTerm::must_be_unbound()? You typically only need that for returning blobs, so it could also be part of a C++ blob infrastructure.
When I was applying my changes to users of SWI-cpp.h, I found a number of places where unification was assumed to succeed. However, Jan told me in a private email that a check is always needed because even when unifying with a newly created term (from PL_new_term_ref(), which is encapsulated in the PlTerm_var constructor), there can be failure, typically from out-of-memory.
So, I annotated the regular unification methods with [[nodiscard]] to catch such errors and added the “must succeed” methods that throw an exception if they fail (with the suffix “_ex”). If my understanding is wrong, I could remove the “_ex” methods, and programmers can use write, e.g., (void)t.unify_term(t2) to turn off the warning.
Too complicated for me 
(As long as I stick with the “safe” subset of C++)