From: William S F. <ws...@fu...> - 2014-01-10 08:01:02
|
On 18/12/13 21:28, Vadim Zeitlin wrote: > Hello, > > I have another weird bug to report: this time it's a regression > introduced by https://github.com/swig/swig/commit/e186d2 which fixed > another bug I reported (speak about irony), see this thread: > http://thread.gmane.org/gmane.comp.programming.swig.devel/22739/ > > The simplest example reproducing the problem I can give is this: > > % cat -n attr.i > 1 %module attr > 2 %include "attribute.i" > 3 %include "boost_shared_ptr.i" > 4 %inline %{ > 5 #include <boost/shared_ptr.hpp> > 6 using namespace boost; > 7 %} > 8 %shared_ptr(Foo); > 9 %feature("naturalvar", 0) boost::shared_ptr<Foo>; > 10 %attributeval(Bar, shared_ptr<Foo>, F, GetFoo) > 11 > 12 %inline %{ > 13 struct Foo { > 14 explicit Foo(int n) : n(n) { printf("Foo(%d)\n", n); } > 15 ~Foo() { printf("~Foo(%d)\n", n); } > 16 > 17 int n; > 18 }; > 19 struct Bar { > 20 explicit Bar(int n) : foo(new Foo(n*n)) { } > 21 shared_ptr<Foo> GetFoo() const { return foo; } > 22 > 23 private: > 24 shared_ptr<Foo> foo; > 25 }; > 26 %} > > This works with e186d21^ but the generated code doesn't compile ever since > then because the naturalvar is not taken into account any more, i.e. SWIG > generates the same wrong > > result = (boost::shared_ptr< Foo > *) &Bar_F_get(arg1); > > line as without %feature(naturalvar, 0) at all. And this code is wrong > because %attributeval uses > > #define Bar_F_get(self_) new shared_ptr<Foo>(self_->GetFoo()) > > to emulate the attribute, and so the generated code fails to compile: > > % ~/build/swig/swig -python -c++ -o wrap_attr_python.cpp attr.i > % g++ -g -shared -fpic -o _attr.so -I/usr/include/python2.6 wrap_attr_python.cpp > wrap_attr_python.cpp: In function 'PyObject* _wrap_Bar_F_get(PyObject*, PyObject*)': > wrap_attr_python.cpp:3405: error: lvalue required as unary '&' operand > > Just for completeness, with e186d21^ the following code is generated: > > result = (boost::shared_ptr< Foo > *)Bar_F_get(arg1); > > which is correct. > > > I still (even in spite of William's explanations the last time) have no > idea about how to fix this without breaking the previously reported bug. > What bothers me is that if I run SWIG with "-debug-module 3", following the > suggestion in the original thread, I see this in the output: > > +++ namespace ---------------------------------------- > | name - "boost" > ... > +++ class ---------------------------------------- > | name - "boost::shared_ptr<(Foo)>" > ... > | feature:naturalvar - "1" > > i.e. it looks that even attaching the feature doesn't actually work. But > the most confusing thing is that this output is exactly the same with > e186d21^, so it looks like my %feature("naturalvar") isn't taken into > account at this level at all -- which is further proved by the fact that > the output of "-debug-module 3" is the same, whether %feature is present or > not in the input file, but the generated code definitely differs. So I > think I'm looking in a completely wrong place and don't have much hope of > finding the problem without some more help... [/me looks at William > inquiringly] > Yes, %feature("naturalvar") is set to 1 for boost::shared_ptr<Foo> and this is correct behaviour. The reason is in line 8, the %shared_ptr macro expands to these notable elements: %feature("naturalvar") Foo; %feature("naturalvar") boost::shared_ptr< Foo >; ... %template() boost::shared_ptr< Foo >; The features are attached to boost::shared_ptr<Foo> when it is instantiated with %template. In line 9 of your code, the attempt to unset the feature is too late as the template is already instantiated. > > In any case, I think it would be better to revert e186d21 than to do > nothing as the other bug should be more rare and could be worked around > (albeit with great difficulty in my real-life case), whereas the current > bug makes it impossible to have shared_ptr-valued attributes completely > which is a show-stopper for me. In fact, I'd like to add a test case > checking that this works to prevent it from being broken again in the > future and will do it if there are no objections if/when the current bug is > fixed. > I think e186d21 is more correct than the previous behaviour. There are workarounds, and other bugs to fix to get to where you want to be. One workaround which is buggy is to change line 9 to: 9 %feature("naturalvar", 0) Bar::F; Here is a patch to fix it which I'll commit soonish: diff --git a/Source/Modules/lang.cxx b/Source/Modules/lang.cxx index c3c148f..3c39d47 100644 --- a/Source/Modules/lang.cxx +++ b/Source/Modules/lang.cxx @@ -477,6 +477,13 @@ void swig_pragma(char *lang, char *name, char *value) { int Language::use_naturalvar_mode(Node *n) const { if (Getattr(n, "unnamed")) return 0; + + // The naturalvar feature can be attached to either the variable name or the variable's type + // naturalvar on the variable name is more specific and overrides naturalvar on the variable's type + String *naturalvar = Getattr(n, "feature:naturalvar"); + if (naturalvar && Strcmp(naturalvar, "0") == 0) + return 0; + and the variable name overriding the variable's type should be documented. This behaviour already exists but for setting rather than unsetting the feature. > Also, just to make things more interesting, there seems to be another bug > in the same area: if "Foo" is used instead of "F" in the line 10, i.e. if I > want to use the same name for the attribute as for the struct, then the > code never works, even with e186d21^ version: SWIG still processes it > without any warnings but naturalvar is not taken into account and the > generated code doesn't compile again. > > As usual, thanks in advance for any help! Sorry, I've only just got around to looking at this. Hopefully the suggested change is okay for you. I'm also pondering other solutions which I'd welcome comments: - make it possible to remove %naturalvar from %shared_ptr in some way. It doesn't feel as that it should be necessary though. I'm not sure how that could be implemented though. Either a preprocessor macro like the ones switching in boost or std for shared_ptr or an additional %shared_ptr macro. - It seems to me that %extend and %naturalvar should work better together. (The %attributeval macro uses %extend). William |