Add useful config options for hydrogen/Qt bindings #160

charbeljc · 2021-06-23T14:56:06Z

Hi, congrats for your work, binder is amazing. Here is a patch adding some configuration options that I added to make bindings for hydrogen

This patch add options to the config file:

-field MyClass::field_y_dont_want
+include_before <qtcleanup.h>
+primitive QString

+include_before allow us to insert headers before <pyind11/pybind11.h> so we can put, eg

#undef slots

and make Python.h happy again when binding something using Qt with binder/pybind11.

-fieldallow us to opt out for a field you don't want
+primitiveallows us to actually generate methods referencing the type without actually binding it. This works provided you have proper type_caster template instantiation in some included header.

Let me know what you think !
Regards,
Charbel

lyskov

Thank you for writing this @charbeljc! Please see a few comments and questions below. Thanks,

lyskov · 2021-06-24T21:27:12Z

source/config.cpp


 	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"};


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

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

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

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.

lyskov · 2021-06-24T21:28:46Z

source/config.cpp

@@ -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"};


+1 I like this idea!

lyskov · 2021-06-24T21:35:08Z

source/config.cpp

+}
+
+
+


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,

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

lyskov · 2021-06-24T21:36:13Z

source/config.cpp


+	string const _primitive_			 {"primitive"};


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,

I just added some docs, see 5d150fd

lyskov · 2021-06-24T21:59:06Z

source/function.cpp

+	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);
+	}


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?

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 😁

lyskov · 2021-06-24T22:00:00Z

source/function.cpp

+	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;
+	}


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

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.

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,

lyskov · 2024-04-13T00:41:38Z

closing due to inactivity

add config options useful for hydrogen bindings

47946db

charbeljc mentioned this pull request Jun 23, 2021

[WIP] python bindings hydrogen-music/hydrogen#1303

Closed

charbeljc changed the title ~~Add useful config options for hydrogen bindings~~ Add useful config options for hydrogen/Qt bindings Jun 23, 2021

lyskov requested changes Jun 24, 2021

View reviewed changes

document new config additions

5d150fd

andriish mentioned this pull request Nov 17, 2021

updating fmt library to 7.1.3 #164

Merged

jlblancoc mentioned this pull request Apr 26, 2023

Add option to skip binding a field #257

Merged

lyskov closed this Apr 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add useful config options for hydrogen/Qt bindings #160

Add useful config options for hydrogen/Qt bindings #160

charbeljc commented Jun 23, 2021 •

edited

Loading

lyskov left a comment

lyskov Jun 24, 2021

charbeljc Jun 26, 2021

lyskov Jun 29, 2021

lyskov Jun 29, 2021

lyskov Jun 24, 2021

lyskov Jun 24, 2021

charbeljc Jun 26, 2021

lyskov Jun 24, 2021

charbeljc Jun 26, 2021

lyskov Jun 24, 2021

charbeljc Jun 26, 2021

lyskov Jun 24, 2021

charbeljc Jun 26, 2021

lyskov Jun 29, 2021

lyskov commented Apr 13, 2024

Add useful config options for hydrogen/Qt bindings #160

Add useful config options for hydrogen/Qt bindings #160

Conversation

charbeljc commented Jun 23, 2021 • edited Loading

lyskov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyskov commented Apr 13, 2024

charbeljc commented Jun 23, 2021 •

edited

Loading