Skip to content

Add Pltermv(0) + clean up initialization checks #31

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.

Sign up for GitHub

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

Closed
kamahen wants to merge 1 commit into from
Closed

Add Pltermv(0) + clean up initialization checks #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 62 additions & 49 deletions SWI-cpp2.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,32 @@ particularly integer conversions.
class PlAtom;
class PlTerm;
class PlTermv;
void PlCheck(int rc);


// A pseudo-exception for quick exist on failure, for use by the unify
// methods. This is special-cased in the PREDICATE et al macros.
// Note that it is *not* a subclass of PlException. See the
// documentation for more details on how this works with returning
// Prolog failure and returning exceptions.
class PlFail
{
public:
explicit PlFail() {}
};


// Throw PlFail on failure or exception. This exception is caught by
// the PREDICATE, which simply returns false ... if the failure was
// caused by an exception, SWI-Prolog will detect that and turn the
// failure into a Prolog exception. Therefore, there is no need for
// calling PL_exception(0) and doing something different if there is
// a pending Prolog exception (to call PL_exception(0), use
// PlException_qid()).
inline void
PlCheck(int rc)
{ if ( !rc )
throw PlFail();
}


/*******************************
Expand All @@ -105,9 +130,8 @@ template <typename C_t> class WrappedC
bool not_null() const { return C_ != null; }
void verify() const; // Throw exception if is_null()

public:
WrappedC<C_t>() : C_(null) { }
explicit WrappedC<C_t>(C_t v) : C_(v) { }
explicit WrappedC<C_t>(C_t v)
: C_(v) { }
WrappedC<C_t>(const WrappedC<C_t>&) = default;
// WrappedC<C_t>& operator =(const WrappedC<C_t>&) = default; // deprecated/deleted in PlTerm
operator bool() const = delete; // Use not_null() instead
Expand Down Expand Up @@ -159,7 +183,8 @@ class PlStringBuffers
class PlFunctor : public WrappedC<functor_t>
{
public:
PlFunctor(functor_t v = null) : WrappedC<functor_t>() { }
PlFunctor(functor_t v)
: WrappedC<functor_t>(v) { }
// PlFunctor(const char*) is handled by std::string constructor
explicit PlFunctor(const std::string& name, size_t arity);
explicit PlFunctor(const std::wstring& name, size_t arity);
Expand All @@ -168,7 +193,10 @@ class PlFunctor : public WrappedC<functor_t>

// TODO: use PlPredicate, PlModule when implemented:
predicate_t pred(module_t m) const {
return PL_pred(C_, m);
predicate_t p = PL_pred(C_, m);
if ( p == nullptr )
throw PlFail();
return p;
}

PlAtom name() const;
Expand All @@ -182,8 +210,8 @@ class PlFunctor : public WrappedC<functor_t>
class PlAtom : public WrappedC<atom_t>
{
public:
PlAtom() : WrappedC<atom_t>() { } // Make constructor public
explicit PlAtom(atom_t v) : WrappedC<atom_t>(v) { }
explicit PlAtom(atom_t v)
: WrappedC<atom_t>(v) { }
explicit PlAtom(const std::string& text) // TODO: add encoding
: WrappedC<atom_t>(PL_new_atom_nchars(text.size(), text.data()))
{ verify();
Expand Down Expand Up @@ -269,30 +297,6 @@ class PlAtom : public WrappedC<atom_t>
* GENERIC PROLOG TERM *
*******************************/


// A pseudo-exception for quick exist on failure, for use by the unify
// methods. This is special-cased in the PREDICATE et al macros.
// Note that it is *not* a subclass of PlException.
class PlFail
{
public:
explicit PlFail() {}
};


// Throw PlFail on failure or exception. This exception is caught by
// the PREDICATE, which simply returns false ... if the failure was
// caused by an exception, SWI-Prolog will detect that and turn the
// failure into a Prolog exception. Therefore, there is no need for
// calling PL_exception(0) and doing something different if there is
// a pending Prolog exception (to call PL_exception(0), use
// PlException_qid()).
inline void
PlCheck(int rc)
{ if ( !rc )
throw PlFail();
}

class PlTerm : public WrappedC<term_t>
{
protected:
Expand All @@ -310,7 +314,8 @@ class PlTerm : public WrappedC<term_t>

public:
PlTerm(const PlTerm&) = default;
explicit PlTerm(const PlAtom& a) : WrappedC<term_t>(PL_new_term_ref())
explicit PlTerm(const PlAtom& a)
: WrappedC<term_t>(PL_new_term_ref())
{ verify();
PlCheck(PL_put_atom(C_, a.C_));
}
Expand Down Expand Up @@ -529,13 +534,14 @@ class PlTerm_atom : public PlTerm
class PlTerm_var : public PlTerm
{
public:
explicit PlTerm_var() : PlTerm() {}
explicit PlTerm_var() { } // PlTerm() calls Pl_new_term_ref()
Copy link
Copy Markdown
Member

@JanWielemaker JanWielemaker Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to call Pl_new_term_ref(). Do I understand correctly that the old implementation called it twice?

Copy link
Copy Markdown
Member Author

@kamahen kamahen Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - the old code was explicitly "calling" the parent constructor (PlTerm()), which in turn "called" the WrappedC<term_t> constructor. It is now implicit.

};

class PlTerm_term_t : public PlTerm
{
public:
explicit PlTerm_term_t(term_t t = null) : PlTerm(t) {}
explicit PlTerm_term_t(term_t t)
: PlTerm(t) {}
};

class PlTerm_integer : public PlTerm
Expand Down Expand Up @@ -575,7 +581,6 @@ class PlTerm_recorded : public PlTerm
explicit PlTerm_recorded(record_t r) { PlCheck(PL_recorded(r, C_)); }
};


/*******************************
* TERM VECTOR *
*******************************/
Expand All @@ -587,21 +592,22 @@ class PlTermv
term_t a0_; // A vector of term_t

public:
explicit PlTermv(size_t n)
explicit PlTermv(size_t n = 0)
: size_(n),
a0_(PL_new_term_refs(static_cast<int>(n)))
{ if ( ! a0_ )
a0_(n ? PL_new_term_refs(static_cast<int>(n)) : PlTerm::null)
{ if ( size_ && a0_ == PlTerm::null )
throw PlFail();
}
explicit PlTermv(size_t n, const PlTerm& t0)
: size_(n),
a0_(t0.C_)
{ if ( ! a0_ )
{ if ( size_ && a0_ == PlTerm::null )
throw PlFail();
}

term_t termv() const
{ return a0_;
{ // Note that a0_ can be PlTerm::null if size_ == 0
return a0_;
}

size_t size() const
Expand Down Expand Up @@ -830,6 +836,7 @@ WrappedC<C_t>::verify() const

inline
PlFunctor::PlFunctor(const std::string& name, size_t arity)
: WrappedC<functor_t>(null)
{ PlAtom a(name);
C_ = PL_new_functor(a.C_, arity);
PL_unregister_atom(a.C_);
Expand All @@ -838,6 +845,7 @@ PlFunctor::PlFunctor(const std::string& name, size_t arity)

inline
PlFunctor::PlFunctor(const std::wstring& name, size_t arity)
: WrappedC<functor_t>(null)
{ PlAtom a(name);
C_ = PL_new_functor(a.C_, arity);
PL_unregister_atom(a.C_);
Expand Down Expand Up @@ -1026,7 +1034,7 @@ class PlFrame

private:
void verify()
{ if ( !fid_ )
{ if ( fid_ == static_cast<fid_t>(0) )
throw PlFail();
}
};
Expand All @@ -1051,16 +1059,21 @@ class PlQuery
}
// TODO: PlQuery(const wstring& ...)
PlQuery(const std::string& name, const PlTermv& av, int flags = PL_Q_PASS_EXCEPTION)
: qid_(PL_open_query(static_cast<module_t>(0), flags,
// TODO: throw if PL_predicate() returns 0
PL_predicate(name.c_str(), static_cast<int>(av.size()), "user"),
: qid_(PL_open_query(static_cast<module_t>(0),
flags,
// TODO: throw if PL_predicate() returns 0
PL_predicate(name.c_str(),
static_cast<int>(av.size()),
"user"), // TODO: static_cast<module_id>(0)
av.termv()))
{ verify();
}
PlQuery(const std::string& module, const std::string& name, const PlTermv& av, int flags = PL_Q_PASS_EXCEPTION)
: qid_(PL_open_query(static_cast<module_t>(0), flags,
// TODO: throw if PL_predicate() returns 0
PL_predicate(name.c_str(), static_cast<int>(av.size()), module.c_str()),
// TODO: throw if PL_predicate() returns 0
PL_predicate(name.c_str(),
static_cast<int>(av.size()),
module.c_str()),
av.termv()))
{ verify();
}
Expand Down Expand Up @@ -1098,7 +1111,7 @@ class PlQuery

private:
void verify()
{ if ( !qid_ )
{ if ( qid_ == static_cast<qid_t>(0) )
throw PlFail();
}
};
Expand Down Expand Up @@ -1314,7 +1327,7 @@ PlCompound::PlCompound(const wchar_t *text)
inline
PlCompound::PlCompound(const std::string& text, PlEncoding enc)
{ term_t t = PL_new_term_ref();
if ( ! t )
if ( t == PlTerm::null )
throw PlFail();

// TODO: PL_put_term_from_chars() should take an unsigned int flags
Expand Down
13 changes: 10 additions & 3 deletions test_cpp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,13 @@ PREDICATE(hello_query, 2)
return true;
}

PREDICATE(call_cut, 1)
{ PlQuery q(A1.as_string(), PlTermv());
PlCheck(q.next_solution());
q.cut();
return true;
}

PREDICATE(hello_call, 1)
{ PlCheck(PlCall(A1));
return true;
Expand Down Expand Up @@ -500,7 +507,7 @@ PREDICATE(ensure_PlTerm_forward_declarations_are_implemented, 0)
PlTerm_atom p_atom4(std::string("abc"));
PlTerm_atom p_atom5(std::wstring(L"世界"));
PlTerm_term_t t_t(PL_new_term_ref());
PlTerm_term_t t_null; // null
PlTerm_term_t t_null(PlTerm::null);
PlTerm_integer t_int1(std::numeric_limits<int>::max());
PlTerm_integer t_int1b(std::numeric_limits<int>::min());
PlTerm_integer t_int2(std::numeric_limits<long>::max());
Expand All @@ -524,7 +531,7 @@ PREDICATE(ensure_PlTerm_forward_declarations_are_implemented, 0)
PlAtom atom3(std::string("atom3"));
PlAtom atom4(std::wstring(L"原子4"));
PlAtom a5a(t_atom1.as_atom());
PlAtom atom_null;
PlAtom atom_null(PlAtom::null);
// The following are unsafe (the as_string() is deleted in the statement):
// const char * x01 = t_var.as_string().c_str();
// const wchar_t *x01a = t_var.as_wstring().c_str();
Expand Down Expand Up @@ -990,7 +997,7 @@ PREDICATE(cpp_options, 3)
int quoted = false;
size_t length = 10;
PlTerm_var callback;
PlAtom token;
PlAtom token(PlAtom::null);
Copy link
Copy Markdown
Member

@JanWielemaker JanWielemaker Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😢

Copy link
Copy Markdown
Member Author

@kamahen kamahen Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@JanWielemaker JanWielemaker Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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++ 😄

const char *descr = "";
bool opt_all_v = opt_all.as_bool();
int flags = opt_all_v ? OPT_ALL : 0;
Expand Down
15 changes: 13 additions & 2 deletions test_cpp.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,9 @@
:- if(\+current_prolog_flag(windows, true)).
% The C++ atom_to_string/2 takes the raw bytes from atom and creates
% a byte string. This doesn't work in Windows as we use a different
% representation. This is dubious anyway. Delete entirely fix when
% the C++ interface properly supports encodings.
% representation. This is dubious anyway.
% TODO: Delete entire fix when the C++ interface properly supports encodings.
% See: https://swi-prolog.discourse.group/t/ann-swi-prolog-9-1-0/5964/3
test(as_string, S == "ä¸\u0096ç\u0095\u008Cå\u009B\u009B") :-
atom_to_string(世界四, S).
test(as_string, S = "hello(ä¸\u0096ç\u0095\u008Cå\u009B\u009B)") :-
Expand Down Expand Up @@ -118,6 +119,16 @@
test(hello_0, Out == "hello world\n") :-
with_output_to(string(Out), hello).

call_cut_test :-
setup_call_cleanup(true,
between(1, 5, _X),
atom_codes(_,_)).

test(call_cut, error(existence_error(procedure,call_cut_test/0))) :-
% This tests that an error in ~PlQuery() is handled properly
% See discussion: https://github.com/SWI-Prolog/packages-cpp/pull/27
call_cut("call_cut_test").

test(term_1, Term = hello(world)) :-
term(Term).

Expand Down