First attempt to bind constants #155
Conversation
There was a problem hiding this comment.
@andriish sorry for the late reply, - too much things on my plate lately! Thank you for adding this! I did quick pass and highlight some things that we will need to address before this could be merged. I will do a few more reads later. Thanks,
source/const.cpp
Outdated
| clang::LangOptions lang_opts; | ||
| lang_opts.CPlusPlus = true; | ||
| clang::PrintingPolicy Policy(lang_opts); | ||
| init->printPretty(rso, NULL, Policy); |
source/const.cpp
Outdated
| std::string type = E->getType().getCanonicalType().getAsString(); | ||
| std::string pytype = ""; | ||
| bool pytype_set = false; | ||
| //This is a list of types that can bi binded with pybind, see https://pybind11.readthedocs.io/en/stable/advanced/pycpp/object.html*/ |
There was a problem hiding this comment.
could you please fix indentation here and remove */ at the line end?
source/const.cpp
Outdated
| // Generate binding for given function: py::enum_<MyEnum>(module, "MyEnum")... | ||
| std::string bind_const(std::string const & module, VarDecl const *E) | ||
| { | ||
| string r=" "; |
There was a problem hiding this comment.
we need to use \t instead of just space to keep indentation correct
source/const.hpp
Outdated
| clang::NamedDecl const * named_decl() const override { return E; }; | ||
|
|
||
| /// check if generator can create binding | ||
| bool bindable() const override; |
There was a problem hiding this comment.
sorry, pedantic: indentation here is broken
test/T60.const.hpp
Outdated
| #include <string> | ||
| #include <vector> | ||
|
|
||
| const int global_int=1; |
There was a problem hiding this comment.
Here and below: could you please fix the const placement here and below to match the rest of source code? Thanks,
test/T60.const.hpp
Outdated
| const std::string global_string2=std::string("Some test string"); | ||
|
|
||
|
|
||
| const double expression_global_double=8.0+1.0/5.0; |
There was a problem hiding this comment.
I see that this will be bound verbatim below as pybind11::float_(8. + 1. / 5.) which is problematic. Consider this: let's say we binding constant for some deep-nested namespace that depend on some other constants like this:
namespace TEST {
int const A = 1;
int const B = A+1
...if binding code for this will be generate as A+1 then result will not compile because expression should really be `TEST::A +``
My recommendation would be to disable bindings for expression unless we can somehow evaluate them (this might be possible though since LLVM have required code)
There was a problem hiding this comment.
Yes, that is a problematic part.
we can somehow evaluate them
Eh, it is safer not to evaluate them. Just imagine a code
#ifdef MYFLAG
int const a=5;
#else
int const a=6;
#endif
and then somewhere binding the constants like
int const i_want_to_bind_it mya = 100 * a;
In case the evaluation would be turned on, one would have to regenerate the binding code each time the 'a' is updated or the MYFLAG is changed.
What could make more sense is to transform the RHS from the "raw" code into a canonical representation. I will have to look how to do that (LLVM should be able to do that, or am I wrong?).
test/T60.const.hpp
Outdated
|
|
||
| const double expression_global_double=8.0+1.0/5.0; | ||
|
|
||
| const double array_global_double[5]={1.0,2.0,3.0,4.0,5.0}; //This should not appear in bindings so far. |
There was a problem hiding this comment.
here and below: could you please add _not_binded suffix to all names thats should not be bound? That will make test script to raise alarm before compilation/linking failure which will be more readable for developers. Thanks,
|
|
||
| int foonamespaced_foo_char(char *) { return 0; } | ||
|
|
||
| } |
There was a problem hiding this comment.
Can we add some user defined data types here? If they can not be bound that totally fine but i want to make sure that Binder is always generate compilable code. Thanks,
There was a problem hiding this comment.
The constants can be bound using the special types like "float_", so I custom types cannot use it, as far as I understand.
| /// extract include needed for this generator and add it to includes vector | ||
| void ConstBinder::add_relevant_includes(IncludeSet &includes) const | ||
| { | ||
| } |
There was a problem hiding this comment.
Why this function is empty, was this just an oversight or we can not deduce includes for some reason? Can we just do binder::add_relevant_includes(E, includes, 0);?
There was a problem hiding this comment.
My understanding was that once only the standard types are used, no extra includes are needed.
And the std::string is included anyway.
source/const.cpp
Outdated
| if ( E->getType().getTypePtr()->isArrayType()) return false; | ||
| if( E->isCXXInstanceMember() or E->isCXXClassMember() ) return false; | ||
| if( E->isCXXInstanceMember() ) return false; | ||
| if ( standard_name( E->getType().getCanonicalType().getAsString() ) == "const std::string" ) return true; | ||
| if (E->getType().getTypePtr()->isRealFloatingType() ) return true; |
There was a problem hiding this comment.
could you please fix formatting here so it consistent? Sorry: i am pedantic...
|
@andriish i am reviewing old PR's that have not been active for a while. Looks like #155 (comment) might be show stopper for this one so i am incline to close this PR until new ideas emerge. Thoughts? |
This is the first attempt to bind constants in binder.
The implementation is in the files const.hpp and const.cpp, which structurally are very similar to enum.hpp and enum.cpp.
The current version binds only the int/float/strings to the types described in https://pybind11.readthedocs.io/en/stable/advanced/pycpp/object.html. So far this is done exclusively for those types as 1) they use python attributes 2) to make sure nothing else is affected.
Potentially this can be extended to more interesting cases of constant vectors and arrays and more generic types.
Best regards,
Andrii