Add Pltermv(0) + clean up initialization checks#31
Add Pltermv(0) + clean up initialization checks#31kamahen wants to merge 1 commit intoSWI-Prolog:masterfrom
Conversation
JanWielemaker
left a comment
There was a problem hiding this comment.
I dislike the PlAtom() constructor. I first thought the PlTerm_var() was a serious mistake, but it looks like my misunderstanding. Just like to get it confirmed. PL_scan_options() is just a remark to keep in mind, not a request to change anything now.
| public: | ||
| PlAtom() : WrappedC<atom_t>() { } // Make constructor public | ||
| explicit PlAtom(atom_t v) : WrappedC<atom_t>(v) { } | ||
| PlAtom() // allow implicit "null" constructor, even though a bit dangerous |
There was a problem hiding this comment.
Why do we want this? Except for crashing it seems there is little useful you can do with this.
There was a problem hiding this comment.
See my other reply -- rocksdb4pl.cpp needs it to make read_optdefs easier to write.
I've pushed a change to rocksdb4pl.cpp - please take a look at it (JanWielemaker/rocksdb#15)
| { | ||
| public: | ||
| explicit PlTerm_var() : PlTerm() {} | ||
| explicit PlTerm_var() { } // PlTerm() calls Pl_new_term_ref() |
There was a problem hiding this comment.
We need to call Pl_new_term_ref(). Do I understand correctly that the old implementation called it twice?
There was a problem hiding this comment.
No - the old code was explicitly "calling" the parent constructor (PlTerm()), which in turn "called" the WrappedC<term_t> constructor. It is now implicit.
| size_t length = 10; | ||
| PlTerm_var callback; | ||
| PlAtom token; | ||
| PlAtom token(PlAtom::null); |
There was a problem hiding this comment.
This is the only place where we want atom_t being 0. You create it explicit, which I think is good. With the added 0-arguments constructor, PlAtom token() would have worked too, no? Overall, PL_scan_options() is an ok interface from C, but I think it needs a serious redesign for C++. Variadic argument functions and this way to use pointers to the inside of the C++ objects is not so nice 😢
There was a problem hiding this comment.
Answering your general comment with a long-winded answer (which I wrote to myself), which perhaps should go into the documentation.
tl;dr: without PlAtom(), we need to put PlAtom(PlAtom::null) in each row of read_opts in rocksdb4pl.cpp.
As a software philosophy, it's important that there should be no such thing as a "half-constructed" object - the constructor should ensure that the object is "complete" or else throw an exception. That is the purpose of PlTerm::verify(), and you can see that I call it in every constructor that instantiates PlTerm.C_ using Pl_new_term_ref(). [Perhaps verify() was a bad choice of name ... maybe change it to verify_else_throw()?]
PlTerm has a problem with overloaded constructors - there's no way to distinguish between PlTerm(int) and PlTerm(term_t). So, I made subclasses that exist only for their constructors. That's the reason for PlTerm_var. (I assume that after t=PL_new_term_ref(), there's no need to do PL_put_variable(t).)
However, all the other objects allow the notion of a "null" object. This is used in rocksdb4pl.cpp to lazily create atoms and predicates (see read_optdefs and pred_call6). I would have preferred to not allow this, and in some cases I don't (e.g., PlModule requires a name; PlFunctor requires either a name+arity or a functor_t). The only reason that PlAtom's "null" constructor is allowed, is to make it easy to write the lookup table for read_optdefs.
There was a problem hiding this comment.
Thanks. I see. I guess ideally we should change APIs such that there is no need for "half baked" objects? Not sure how feasible this is. And yes, PL_new_term_ref() creates a term that is unbound. Note that the Quintus and SICStus counterparts create a term that is bound to [].
I'll wait for you to complete this sequence of fast commits and will merge then. By now it seems you understand all the dirty corners of SWI-Prolog's C interface and you definitely understand more of C++ 😄
- also removed some unused parameter defaults
|
Squashed with #32 |
Fixing the PL_predicate() call turned out to be more work than expected (there's a TODO there), so will be in a future PR