Thread: [pygccxml-development] Implicit virtual functions in derived classes
Brought to you by:
mbaas,
roman_yakovenko
From: Julian S. <jul...@rs...> - 2008-02-15 01:23:22
|
Hi, I ran into the following problem with Py++ 0.9.0. Consider this class hierarchy: class A { public: virtual void foo() {} }; class B: public A { }; This will not generate a wrapper for B even though B inherits A's virtual function. Now if I have the following Python code: class C(B): def foo(self): print "C.foo" ... then when foo() is invoked on this instance on the C++ side of things, the Python code won't be executed as the wrapper is missing. I managed to force generation of a wrapper for B, but even then it won't generate the necessary override code (virtual void foo, void default_foo, and the registration code.) My workaround is currently to manually add any implicit virtual functions that I need to override, but this is cumbersome and error-prone. Is there a way to configure or patch Py++ so that this is not necessary? Thanks in advance, .Julian |
From: Roman Y. <rom...@gm...> - 2008-02-15 20:54:46
|
On Fri, Feb 15, 2008 at 3:22 AM, Julian Scheid <jul...@rs...> wrote: > Hi, > > I ran into the following problem with Py++ 0.9.0. > > Consider this class hierarchy: > > class A > { > public: > virtual void foo() {} > }; > > class B: public A > { > }; > > > This will not generate a wrapper for B even though B inherits A's > virtual function. Now if I have the following Python code: > > class C(B): > def foo(self): > print "C.foo" > > ... then when foo() is invoked on this instance on the C++ side of > things, the Python code won't be executed as the wrapper is missing. > > I managed to force generation of a wrapper for B, but even then it won't > generate the necessary override code (virtual void foo, void > default_foo, and the registration code.) > > My workaround is currently to manually add any implicit virtual > functions that I need to override, but this is cumbersome and > error-prone. Is there a way to configure or patch Py++ so that this is > not necessary? Thank you very much for bug reporting. Few minutes ago I committed fix and added new test case. The whole revision content: http://pygccxml.svn.sourceforge.net/viewvc/pygccxml?view=rev&revision=1238 The fix: http://pygccxml.svn.sourceforge.net/viewvc/pygccxml/pyplusplus_dev/pyplusplus/decl_wrappers/class_wrapper.py?r1=1238&r2=1237&pathrev=1238 I tested the fix again Py++ 0.9.5, but I guess it will also work for your version too. -- Roman Yakovenko C++ Python language binding http://www.language-binding.net/ |
From: Julian S. <jul...@rs...> - 2008-02-16 01:06:25
|
Roman Yakovenko wrote: >> This will not generate a wrapper for B even though B inherits A's >> virtual function. Now if I have the following Python code: >> >> class C(B): >> def foo(self): >> print "C.foo" >> >> ... then when foo() is invoked on this instance on the C++ side of >> things, the Python code won't be executed as the wrapper is missing. > > Thank you very much for bug reporting. Few minutes ago I committed fix > and added new test case. > > The whole revision content: > http://pygccxml.svn.sourceforge.net/viewvc/pygccxml?view=rev&revision=1238 > > The fix: > http://pygccxml.svn.sourceforge.net/viewvc/pygccxml/pyplusplus_dev/pyplusplus/decl_wrappers/class_wrapper.py?r1=1238&r2=1237&pathrev=1238 > > I tested the fix again Py++ 0.9.5, but I guess it will also work for > your version too. Thanks for your quick response. In the meantime I arrived at a similar solution but I found that just adding non-pure virtual functions to the "redefined_funcs" list alone, as in your patch, didn't do the trick. In addition, I had to do the following (this is against Py++ 0.9.0): - Make sure that the original function is not ignored and is exportable - Make sure it is not static (weird, I know, because static methods should never be virtual - yet for some reason a few static methods ended up in the query results.) - Make sure the original function is actually defined in the base class and not in an inner class of the base class. So my code looks roughly like this: funcs.update( base_cls.member_functions( query | (all_virtual & declarations.custom_matcher_t( lambda decl: decl.parent.alias == base_cls.alias and not decl.has_static and not decl.ignore and decl.exportable), allow_empty=True ) ) I might have a bug at another point in my generate_code.py so take this with a grain of salt - not sure why all of this is necessary. I'll try again with your HEAD and let you know how I go. Just a heads up that possibly it's not that trivial. I'm also not sure whether your only including public virtual functions is correct. Unless I'm missing something, if you declare the "foo" function in my original example protected instead of public you'd still expect it to be overridable. Thanks, Julian |
From: Julian S. <jul...@rs...> - 2008-02-16 05:44:55
|
Julian Scheid wrote: > funcs.update( base_cls.member_functions( query | (all_virtual & > declarations.custom_matcher_t( lambda decl: decl.parent.alias == > base_cls.alias and not decl.has_static and not decl.ignore and > decl.exportable), allow_empty=True ) ) I found one more issue with my fix and I think your patch suffers from the same problem. Consider this situation: class A { public: virtual void foo() {} }; class B: public A { public: virtual void foo() {} }; class C: public B { } Due to the way funcs.update works (?) this will put A.foo into "funcs" instead of B.foo when generating the wrapper for C. This in turn will lead to the following wrapper code for class C to be generated: virtual void foo( ) { if( bp::override func_foo = this->get_override( "foo" ) ) { func_foo( ); } else this->A::foo( ); } Note it generates "this->A::foo" in the end where it actually should invoke this->B:foo. I haven't yet found a solution, simply reversing the base class list used in the outer for loop doesn't seem to be enough. Julian |
From: Roman Y. <rom...@gm...> - 2008-02-16 07:41:35
|
On Sat, Feb 16, 2008 at 3:06 AM, Julian Scheid <jul...@rs...> wrote: > I'm also not sure whether your only including public virtual functions > is correct. Unless I'm missing something, if you declare the "foo" > function in my original example protected instead of public you'd still > expect it to be overridable. The code we are dealing with, in this problem is very very fragile. So I prefer to enhanced it only for use cases I have real test cases. -- Roman Yakovenko C++ Python language binding http://www.language-binding.net/ |
From: Julian S. <jul...@rs...> - 2008-02-16 01:29:32
|
Roman Yakovenko wrote: > Thank you very much for bug reporting. Few minutes ago I committed fix > and added new test case. > > The whole revision content: > http://pygccxml.svn.sourceforge.net/viewvc/pygccxml?view=rev&revision=1238 > > The fix: > http://pygccxml.svn.sourceforge.net/viewvc/pygccxml/pyplusplus_dev/pyplusplus/decl_wrappers/class_wrapper.py?r1=1238&r2=1237&pathrev=1238 > > I tested the fix again Py++ 0.9.5, but I guess it will also work for > your version too. There's also another problem with this approach (both your patch and my similar fix): the API I'm wrapping is a rather large class hierarchy, and nearly all classes are derived from a common base class with a large number of virtual functions. Let's say for the sake of argument that it's 100 classes and 10 virtual functions in the common base class. This leads to 1000 times the wrapper functions generated, with almost identical code in each wrapper. I found that my Py++ wrapper library, which is already quite large at roughly 20 MB, gets blown up to over 40 MB once I include the base classes' virtual functions. I wonder if it would be possible to derive wrappers from the base class wrapper to help reduce this overhead? E.g. in my example, derive B_wrapper from A_wrapper? Ideally you could also reuse the registration code so that register_B_class would call something like register_A_class_virtuals. Thanks, Julian |
From: Julian S. <jul...@rs...> - 2008-02-16 06:58:20
Attachments:
class_wrapper_derived_virtuals.patch
|
Julian Scheid wrote: > Due to the way funcs.update works (?) this will put A.foo into "funcs" > instead of B.foo when generating the wrapper for C. This in turn will > lead to the following wrapper code for class C to be generated: To elaborate, the order in which "A::foo" and "B::foo" are traversed in the "for f in funcs" loop depends on details of Python's Hashset implementation - they might or might not be traversed in the right order depending on how they are hashed. The right order being B::foo first and then A::foo second so that B::foo is added and A::foo is later rejected when tested with is_same_function. I haven't actually checked how things turn out for my hypothetical example from the previous mail, if you can't reproduce it using that example try using different class names (swapping A and B so that A is derived from B and C is derived from A should do the trick). I found that this problem can be avoided simply by using a list instead of a set for "funcs" and extend'ing it instead of update'ing it. While possibly a tad less efficient, this seems to work fine and guarantees that later on the right superclass is used. There is another intricacy. In the previous example (with classes A, B and C) consider that mb.class_("B").member_functions("foo").exclude() has been called. Now, if you use "not ignored and exportable" in the query as I suggested originally, what happens is that only A.foo ends up in "funcs" and the generated code for C_wrapper will incorrectly dispatch to A::foo. What works better is either to disregard whether or not a function is marked as ignore (downside being that exclude won't affect derived classes) or, better, to filter the resulting list "functions" at the very end of "redefined_funcs" and remove all methods that are marked as ignore. My final results are attached as a patch. It seems to solve all issues except for the overhead problem. (I was wrong about the static modifier - this turned out not to be necessary.) Cheers, Julian |
From: Roman Y. <rom...@gm...> - 2008-02-16 07:46:34
|
On Sat, Feb 16, 2008 at 3:29 AM, Julian Scheid <jul...@rs...> wrote: > There's also another problem with this approach (both your patch and my > similar fix): the API I'm wrapping is a rather large class hierarchy, > and nearly all classes are derived from a common base class with a large > number of virtual functions. > > Let's say for the sake of argument that it's 100 classes and 10 virtual > functions in the common base class. This leads to 1000 times the wrapper > functions generated, with almost identical code in each wrapper. I found > that my Py++ wrapper library, which is already quite large at roughly 20 > MB, gets blown up to over 40 MB once I include the base classes' virtual > functions. > > I wonder if it would be possible to derive wrappers from the base class > wrapper to help reduce this overhead? E.g. in my example, derive > B_wrapper from A_wrapper? Ideally you could also reuse the registration > code so that register_B_class would call something like > register_A_class_virtuals. I should think about this. For some reason I thought that it could not be done without additional code provided by user. Let me check this one more time. -- Roman Yakovenko C++ Python language binding http://www.language-binding.net/ |
From: Roman Y. <rom...@gm...> - 2008-02-17 17:30:10
|
On Feb 16, 2008 9:46 AM, Roman Yakovenko <rom...@gm...> wrote: > On Sat, Feb 16, 2008 at 3:29 AM, Julian Scheid <jul...@rs...> wrote: > > There's also another problem with this approach (both your patch and my > > similar fix): the API I'm wrapping is a rather large class hierarchy, > > and nearly all classes are derived from a common base class with a large > > number of virtual functions. > > > > Let's say for the sake of argument that it's 100 classes and 10 virtual > > functions in the common base class. This leads to 1000 times the wrapper > > functions generated, with almost identical code in each wrapper. I found > > that my Py++ wrapper library, which is already quite large at roughly 20 > > MB, gets blown up to over 40 MB once I include the base classes' virtual > > functions. > > > > I wonder if it would be possible to derive wrappers from the base class > > wrapper to help reduce this overhead? E.g. in my example, derive > > B_wrapper from A_wrapper? Ideally you could also reuse the registration > > code so that register_B_class would call something like > > register_A_class_virtuals. > > I should think about this. For some reason I thought that it could not > be done without > additional code provided by user. > > Let me check this one more time. I just checked this. It is very easy to patch Py++ to generate code in a new way, but after that .... Well, basically the compilation will fail on ambiguities. I attach source file, generated code and errors. If you think, I did it wrong or you will find a way that will make boost.python happy, than I will integrate your solution to Py++ :-) -- Roman Yakovenko C++ Python language binding http://www.language-binding.net/ |
From: Roman Y. <rom...@gm...> - 2008-02-16 07:56:31
|
On Sat, Feb 16, 2008 at 8:58 AM, Julian Scheid <jul...@rs...> wrote: > Julian Scheid wrote: > > Due to the way funcs.update works (?) this will put A.foo into "funcs" > > instead of B.foo when generating the wrapper for C. This in turn will > > lead to the following wrapper code for class C to be generated: > > To elaborate, the order in which "A::foo" and "B::foo" are traversed in > the "for f in funcs" loop depends on details of Python's Hashset > implementation - they might or might not be traversed in the right order > depending on how they are hashed. The right order being B::foo first and > then A::foo second so that B::foo is added and A::foo is later rejected > when tested with is_same_function. > > I haven't actually checked how things turn out for my hypothetical > example from the previous mail, if you can't reproduce it using that > example try using different class names (swapping A and B so that A is > derived from B and C is derived from A should do the trick). > > I found that this problem can be avoided simply by using a list instead > of a set for "funcs" and extend'ing it instead of update'ing it. While > possibly a tad less efficient, this seems to work fine and guarantees > that later on the right superclass is used. > > There is another intricacy. In the previous example (with classes A, B > and C) consider that mb.class_("B").member_functions("foo").exclude() > has been called. Now, if you use "not ignored and exportable" in the > query as I suggested originally, what happens is that only A.foo ends up > in "funcs" and the generated code for C_wrapper will incorrectly > dispatch to A::foo. What works better is either to disregard whether or > not a function is marked as ignore (downside being that exclude won't > affect derived classes) or, better, to filter the resulting list > "functions" at the very end of "redefined_funcs" and remove all methods > that are marked as ignore. > > My final results are attached as a patch. It seems to solve all issues > except for the overhead problem. (I was wrong about the static modifier > - this turned out not to be necessary.) > > Cheers, > > Julian > > --- class_wrapper.py 2008-02-16 17:52:10.000000000 +1100 > +++ class_wrapper_derived_virtuals.py 2008-02-16 17:53:30.000000000 +1100 > @@ -442,19 +442,20 @@ > all_included = declarations.custom_matcher_t( lambda decl: decl.ignore == False and decl.exportable ) > all_protected = declarations.access_type_matcher_t( 'protected' ) & all_included > all_pure_virtual = declarations.virtuality_type_matcher_t( VIRTUALITY_TYPES.PURE_VIRTUAL ) > - all_not_pure_virtual = ~all_pure_virtual > + all_virtual = declarations.virtuality_type_matcher_t( VIRTUALITY_TYPES.VIRTUAL ) & (declarations.access_type_matcher_t( 'public' ) | declarations.access_type_matcher_t( 'protected' )) > + all_not_pure_virtual = ~(all_pure_virtual | all_virtual) > > query = all_protected | all_pure_virtual > relevant_opers = declarations.custom_matcher_t( lambda decl: decl.symbol in ('()', '[]') ) > - funcs = set() > + funcs = [] > defined_funcs = set() > > for base in self.recursive_bases: > if base.access == ACCESS_TYPES.PRIVATE: > continue > base_cls = base.related_class > - funcs.update( base_cls.member_functions( query, allow_empty=True ) ) > - funcs.update( base_cls.member_operators( relevant_opers & query, allow_empty=True ) ) > + funcs.extend( base_cls.member_functions( query | (all_virtual & declarations.custom_matcher_t( lambda decl: decl.parent.alias == base_cls.alias )), allow_empty=True ) ) > + funcs.extend( base_cls.member_operators( relevant_opers & query, allow_empty=True ) ) > > defined_funcs.update( base_cls.member_functions( all_not_pure_virtual, allow_empty=True ) ) > defined_funcs.update( base_cls.member_operators( all_not_pure_virtual & relevant_opers, allow_empty=True ) ) > @@ -481,7 +482,7 @@ > break > else: > not_reimplemented_funcs.add( f ) > - functions = list( not_reimplemented_funcs ) > + functions = filter( lambda f: not f.ignore and f.exportable, list( not_reimplemented_funcs ) ) > functions.sort( cmp=lambda f1, f2: cmp( ( f1.name, f1.location.as_tuple() ) > , ( f2.name, f2.location.as_tuple() ) ) ) > self._redefined_funcs = functions > > Thank you very much!!! I am learning your patch and run through all my tests. If everything is okey I will commit it today. -- Roman Yakovenko C++ Python language binding http://www.language-binding.net/ |
From: Roman Y. <rom...@gm...> - 2008-02-16 18:52:22
|
On Feb 16, 2008 9:56 AM, Roman Yakovenko <rom...@gm...> wrote: > > On Sat, Feb 16, 2008 at 8:58 AM, Julian Scheid <jul...@rs...> wrote: > > Julian Scheid wrote: > > > Due to the way funcs.update works (?) this will put A.foo into "funcs" > > > instead of B.foo when generating the wrapper for C. This in turn will > > > lead to the following wrapper code for class C to be generated: > > > > To elaborate, the order in which "A::foo" and "B::foo" are traversed in > > the "for f in funcs" loop depends on details of Python's Hashset > > implementation - they might or might not be traversed in the right order > > depending on how they are hashed. The right order being B::foo first and > > then A::foo second so that B::foo is added and A::foo is later rejected > > when tested with is_same_function. > > > > I haven't actually checked how things turn out for my hypothetical > > example from the previous mail, if you can't reproduce it using that > > example try using different class names (swapping A and B so that A is > > derived from B and C is derived from A should do the trick). > > > > I found that this problem can be avoided simply by using a list instead > > of a set for "funcs" and extend'ing it instead of update'ing it. While > > possibly a tad less efficient, this seems to work fine and guarantees > > that later on the right superclass is used. > > > > There is another intricacy. In the previous example (with classes A, B > > and C) consider that mb.class_("B").member_functions("foo").exclude() > > has been called. Now, if you use "not ignored and exportable" in the > > query as I suggested originally, what happens is that only A.foo ends up > > in "funcs" and the generated code for C_wrapper will incorrectly > > dispatch to A::foo. What works better is either to disregard whether or > > not a function is marked as ignore (downside being that exclude won't > > affect derived classes) or, better, to filter the resulting list > > "functions" at the very end of "redefined_funcs" and remove all methods > > that are marked as ignore. > > > > My final results are attached as a patch. It seems to solve all issues > > except for the overhead problem. (I was wrong about the static modifier > > - this turned out not to be necessary.) > > > > Cheers, > > > > Julian > > > > --- class_wrapper.py 2008-02-16 17:52:10.000000000 +1100 > > +++ class_wrapper_derived_virtuals.py 2008-02-16 17:53:30.000000000 +1100 > > @@ -442,19 +442,20 @@ > > all_included = declarations.custom_matcher_t( lambda decl: decl.ignore == False and decl.exportable ) > > all_protected = declarations.access_type_matcher_t( 'protected' ) & all_included > > all_pure_virtual = declarations.virtuality_type_matcher_t( VIRTUALITY_TYPES.PURE_VIRTUAL ) > > - all_not_pure_virtual = ~all_pure_virtual > > + all_virtual = declarations.virtuality_type_matcher_t( VIRTUALITY_TYPES.VIRTUAL ) & (declarations.access_type_matcher_t( 'public' ) | declarations.access_type_matcher_t( 'protected' )) > > + all_not_pure_virtual = ~(all_pure_virtual | all_virtual) > > > > query = all_protected | all_pure_virtual > > relevant_opers = declarations.custom_matcher_t( lambda decl: decl.symbol in ('()', '[]') ) > > - funcs = set() > > + funcs = [] > > defined_funcs = set() > > > > for base in self.recursive_bases: > > if base.access == ACCESS_TYPES.PRIVATE: > > continue > > base_cls = base.related_class > > - funcs.update( base_cls.member_functions( query, allow_empty=True ) ) > > - funcs.update( base_cls.member_operators( relevant_opers & query, allow_empty=True ) ) > > + funcs.extend( base_cls.member_functions( query | (all_virtual & declarations.custom_matcher_t( lambda decl: decl.parent.alias == base_cls.alias )), allow_empty=True ) ) > > + funcs.extend( base_cls.member_operators( relevant_opers & query, allow_empty=True ) ) > > > > defined_funcs.update( base_cls.member_functions( all_not_pure_virtual, allow_empty=True ) ) > > defined_funcs.update( base_cls.member_operators( all_not_pure_virtual & relevant_opers, allow_empty=True ) ) > > @@ -481,7 +482,7 @@ > > break > > else: > > not_reimplemented_funcs.add( f ) > > - functions = list( not_reimplemented_funcs ) > > + functions = filter( lambda f: not f.ignore and f.exportable, list( not_reimplemented_funcs ) ) > > functions.sort( cmp=lambda f1, f2: cmp( ( f1.name, f1.location.as_tuple() ) > > , ( f2.name, f2.location.as_tuple() ) ) ) > > self._redefined_funcs = functions > > > > > > Thank you very much!!! > > I am learning your patch and run through all my tests. If everything > is okey I will commit it today. Can I ask you to submit some kind of unit test - something very simple, that will map your class hierarchy. I would like to be sure next time I touch the code the things will continue to work. Thanks -- Roman Yakovenko C++ Python language binding http://www.language-binding.net/ |