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.

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

Jump to bottom

Add useful config options for hydrogen/Qt bindings #160

Closed
wants to merge 2 commits into from
Closed

Add useful config options for hydrogen/Qt bindings #160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47946db
add config options useful for hydrogen bindings
charbeljc Jun 23, 2021
5d150fd
document new config additions
charbeljc Jun 26, 2021
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
50 changes: 50 additions & 0 deletions documentation/config.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,14 @@ Config file directives:



* ``-field``, specify that particular field should be excluded from bindings.

.. code-block:: bash

-field MyClass::some_field



* ``include``, directive to control C++ include directives. Force Binder to either skip adding particular include into generated
source files (``-`` prefix) or force Binder to always add some include files into each generated file. Normally Binder could
automatically determine which C++ header files is needed in order to specify type/functions but in some cases it might be
Expand Down Expand Up @@ -115,6 +123,21 @@ Config file directives:

+include_for_namespace aaaa::bbbb <aaaa/bbbb/namespace_binding.hpp>

* ``include_before``, directive to control C++ include directives to be inserted *before* ``<pybind11/pybind11.h>`` so that you have the
opportunity to cleanup defines introduced by other includes before the inclusion of ``<Python.h>``. This is for instance useful when dealing
with ``Qt`` libraries, which define ``slots``.

.. code-block:: bash

+include_before <qtcleanup.hpp>



.. code-block:: c++

// qtcleanup.hpp
#undef slots



* ``binder``, specify custom binding function for particular concrete or template class. In the example below all
Expand Down Expand Up @@ -160,6 +183,33 @@ Config file directives:



* ``+primitive``: specify that a given class should be treated as a primitive type like ``std::string``, that is, no bindings should be generated
for this class, but functions depending on it as parameters or return value should still be included in class bindings. The developper must then
provide appropriate type casters for this class.

.. code-block:: bash

-class QString
+primitive QString
+include <qstring_caster.hpp>
+include_before <qtcleanup.hpp>


.. code-block:: c++

#include <QtCore/QString>

class MyClass {

public:
...
void use_some_qstring(QString param);
QString get_some_qstring();
...
}



* ``default_static_pointer_return_value_policy``, specify return value policy for static functions returning pointer to objects. Default is
`pybind11::return_value_policy::automatic`.

Expand Down
8 changes: 8 additions & 0 deletions source/class.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,10 @@ string binding_public_data_members(CXXRecordDecl const *C)
for(auto s = u->shadow_begin(); s != u->shadow_end(); ++s) {
if(UsingShadowDecl *us = dyn_cast<UsingShadowDecl>(*s) ) {
if( FieldDecl *f = dyn_cast<FieldDecl>( us->getTargetDecl() ) ) {
auto config = Config::get();
if ( config.is_field_skipping_requested(f->getQualifiedNameAsString())) {
continue;
}
if( is_bindable(f) ) c += "\tcl" + bind_data_member(f, class_qualified_name(C)) + ";\n";
}
}
Expand All @@ -541,6 +545,10 @@ string binding_public_data_members(CXXRecordDecl const *C)

for(auto d = C->decls_begin(); d != C->decls_end(); ++d) {
if(FieldDecl *f = dyn_cast<FieldDecl>(*d) ) {
auto config = Config::get();
if ( config.is_field_skipping_requested(f->getQualifiedNameAsString()) ) {
continue;
}
if( f->getAccess() == AS_public and is_bindable(f) ) c += "\tcl" + bind_data_member(f, class_qualified_name(C)) + ";\n";
}
}
Expand Down
48 changes: 47 additions & 1 deletion source/config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,14 @@ void Config::read(string const &file_name)
string const _namespace_ {"namespace"};
string const _function_ {"function"};
string const _class_ {"class"};
string const _field_ {"field"};
Copy link
Member

Choose a reason for hiding this comment

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

+1 I like this idea!


string const _include_ {"include"};
string const _include_for_class_ {"include_for_class"};
string const _include_for_namespace_ {"include_for_namespace"};
string const _include_before_ {"include_before"};
Copy link
Member

Choose a reason for hiding this comment

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

Could you elaborate why this option is needed? Particularly how is it different from +include directive?

Copy link
Author

Choose a reason for hiding this comment

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

Could you elaborate why this option is needed? Particularly how is it different from +include directive?

All other include directives are written by the code generator after <pybind11/pybind11.h>, which includes <Python.h>.

Since I'm creating bindings for a library that makes use of Qt I needed a way to cleanup the preprocessor namespace before the inclusion of <pybind11/pybind11.h> in the generated code (Qt defines slots for object signal management).

That's the purpose of the +include_before directive.

+include_before <qtcleanup.hpp>
// qtcleanup.hpp
#undef slots

Copy link
Member

Choose a reason for hiding this comment

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

All other include directives are written by the code generator after <pybind11/pybind11.h>, which includes <Python.h>.

Hm... this can't be right: all custom includes is always put on top of the file. For example please see test output here: All other include directives are written by the code generator after <pybind11/pybind11.h>, which includes <Python.h>.
https://github.com/RosettaCommons/binder/blob/master/test/T00.basic.ref#L9

This is also explicitly tested here: https://github.com/RosettaCommons/binder/blob/master/test/T31.include_for_class.ref#L2

Copy link
Member

Choose a reason for hiding this comment

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

Also, please note that current implementation of include_before is no different from include so it would be really surprising if end results would be any different.


string const _primitive_ {"primitive"};
Copy link
Member

Choose a reason for hiding this comment

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

For all new config options: could you please add short short documentation? Good place to put it will be this file: https://github.com/RosettaCommons/binder/blob/master/documentation/config.rst Thanks,

Copy link
Author

Choose a reason for hiding this comment

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

I just added some docs, see 5d150fd

string const _binder_ {"binder"};
string const _add_on_binder_ {"add_on_binder"};

Expand Down Expand Up @@ -120,7 +123,18 @@ void Config::read(string const &file_name)

if(bind) includes_to_add.push_back(name_without_spaces);
else includes_to_skip.push_back(name_without_spaces);

} else if ( token == _include_before_ ) {
if (bind) {
includes_to_add_before.push_back(name_without_spaces);
}
} else if ( token == _primitive_ ) {
if (bind) {
primitives.push_back(name_without_spaces);
}
} else if ( token == _field_ ) {
if (!bind) {
fields_to_skip.push_back(name_without_spaces);
}
} else if( token == _include_for_class_ ) {

if(bind) {
Expand Down Expand Up @@ -302,6 +316,23 @@ bool Config::is_class_skipping_requested(string const &class__) const
}


bool Config::is_field_skipping_requested(string const &field_) const
{
string field {field_};
field.erase(std::remove(field.begin(), field.end(), ' '), field.end());

auto bind = std::find(fields_to_skip.begin(), fields_to_skip.end(), field);

if( bind != fields_to_skip.end() ) {
//outs() << "Skipping: " << class_ << "\n";
return true;
}

return false;
}



Copy link
Member

Choose a reason for hiding this comment

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

For field and for other new options: could you please add simple tests? All tests files are located here: https://github.com/RosettaCommons/binder/tree/master/test . This will also work as minimal example. Since we going to test config options than these test(s) will need .config files. If you can't generate .ref files on CentOS-7 then commit files from your working machine and i will update them later to match output on reference platform. Thanks,

Copy link
Author

Choose a reason for hiding this comment

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

I will look at this and try to write some tests.

bool Config::is_include_skipping_requested(string const &include) const
{
for(auto & i : includes_to_skip)
Expand All @@ -318,4 +349,19 @@ string Config::includes_code() const
return c.size() ? c+'\n' : c;
}


string Config::includes_before_code() const
{
string c;
for(auto & i: includes_to_add_before) c += "#include " + i + "\n";
return c.size() ? c+'\n' : c;
}

bool Config::is_primitive(string const &name) const
{
auto prim = std::find(primitives.begin(), primitives.end(), name);
if ( prim != primitives.end() ) return true;
return false;
}

} // namespace binder
6 changes: 5 additions & 1 deletion source/config.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ class Config

std::vector<string> namespaces_to_bind, classes_to_bind, functions_to_bind,
namespaces_to_skip, classes_to_skip, functions_to_skip,
includes_to_add, includes_to_skip;
includes_to_add, includes_to_skip,
includes_to_add_before, primitives, fields_to_skip;

std::map<string, string> const &binders() const { return binders_; }
std::map<string, string> const &add_on_binders() const { return add_on_binders_; }
Expand Down Expand Up @@ -96,6 +97,9 @@ class Config
bool is_include_skipping_requested(string const &include) const;

string includes_code() const;
string includes_before_code() const;
bool is_primitive(string const &type_) const;
bool is_field_skipping_requested(string const &name) const;
};


Expand Down
6 changes: 5 additions & 1 deletion source/context.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,10 @@ void Context::bind(Config const &config)

for(auto & sp : binders) {
Binder & b( *sp );
if ( config.is_primitive( b.id() ) ) {
flag=false;
continue;
}
if( !b.is_binded() and b.bindable() and b.binding_requested() ) {
//outs() << "Binding: " << b.id() /*named_decl()->getQualifiedNameAsString()*/ << "\n";
b.bind(*this);
Expand Down Expand Up @@ -476,7 +480,7 @@ void Context::generate(Config const &config)

if( i < binders.size() ) --i;

code = generate_include_directives(includes) + fmt::format(module_header, config.includes_code()) + prefix_code + "void " + function_name + module_function_suffix + "\n{\n" + code + "}\n";
code = generate_include_directives(includes) + config.includes_before_code() + fmt::format(module_header, config.includes_code()) + prefix_code + "void " + function_name + module_function_suffix + "\n{\n" + code + "}\n";

if( O_single_file ) root_module_file_handle << "// File: " << file_name << '\n' << code << "\n\n";
else update_source_file(config.prefix, file_name, code);
Expand Down
24 changes: 20 additions & 4 deletions source/function.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,8 +269,14 @@ bool is_binding_requested(FunctionDecl const *F, Config const &config)
{
bool bind = config.is_function_binding_requested( F->getQualifiedNameAsString() ) or config.is_function_binding_requested( function_qualified_name(F) ) or config.is_namespace_binding_requested( namespace_from_named_decl(F) );

for(auto & t : get_type_dependencies(F) ) bind |= binder::is_binding_requested(t, config);

for(auto & t : get_type_dependencies(F) ) {
bool requested = binder::is_binding_requested(t, config);
bool primitive = binder::is_primitive(t, config);
if ( primitive ) {
requested = false;
}
bind |= requested; // binder::is_binding_requested(t, config);
}
Comment on lines +272 to +279
Copy link
Member

Choose a reason for hiding this comment

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

Could you please elaborate of what exactly the primitive option control? After reading PR page i was under impression that this will be used on types what we do not want to bind but rather assume that they ready bound, is that so? If yes, then could you please elaborate why do we need to to check for primitive here?

Also, if above assumption about what primitive does is correct then could you please argue why current implementation is better then adding check to https://github.com/RosettaCommons/binder/blob/master/source/type.cpp#L559 function?

Copy link
Author

Choose a reason for hiding this comment

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

That's It, thanks for the pointer. I'm pretty new to your codebase and I missed it. I will redo this part after I have added some tests 😁

return bind;
}

Expand All @@ -296,8 +302,18 @@ bool is_skipping_requested(FunctionDecl const *F, Config const &config)
}
//outs() << "OK\n";

for(auto & t : get_type_dependencies(F) ) skip |= is_skipping_requested(t, config);

for(auto & t : get_type_dependencies(F) ) {
bool dep_skip = is_skipping_requested(t, config);
bool primitive = is_primitive(t, config);
if ( primitive ) dep_skip = false;
skip |= dep_skip;
}
Comment on lines +305 to +310
Copy link
Member

Choose a reason for hiding this comment

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

again: could you please elaborate why do we need to take primitive into account?

Copy link
Author

Choose a reason for hiding this comment

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

Hum, I was borrowing the +primitive terminology from shiboken, the pyside binding generator. In the light of your code pointer, renaming this to +builtin should be nicer, I guess.

Copy link
Member

Choose a reason for hiding this comment

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

I guess my previous comment might have been a bit cryptic, what i meant ask is this: why do we need to check if type t is build-in? I think we do not since purpose of this function is to determent if binding of given function was explicitly disabled by user. Am i missing something here? If so could you please elaborate? Thanks,

// if (skip) {
// // check if explicitly added
// if (config.is_function_binding_requested(F->getQualifiedNameAsString())) {
// skip = false;
// }
// }
return skip;
}

Expand Down
7 changes: 7 additions & 0 deletions source/type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,4 +660,11 @@ bool is_banned_symbol(clang::NamedDecl const *D)
// return false;
// }

bool is_primitive(const clang::QualType &qt, const Config &config) {
auto id = qt.getBaseTypeIdentifier();
if ( ! id ) return false;
auto name = id->getName();
return config.is_primitive( name.str() );
};

} // namespace binder
2 changes: 1 addition & 1 deletion source/type.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ bool is_banned_symbol(clang::NamedDecl const *D);
// check if class/struct is in banned type lists
//bool is_banned_type(clang::CXXRecordDecl const *C);


bool is_primitive(clang::QualType const &qt, const Config &config);
} // namespace binder

#endif // _INCLUDED_type_hpp_