From: Geert J. <in...@ko...> - 2013-04-19 18:07:21
|
Hi, I'm working through the failing testcases for guile. One testcase fails with a compilation error in some generated code based on std_map and std_pair. I know exactly where this generated code comes from, but my C++ knowledge is insufficient to understand the error, let alone remedy it. I gather some kind of const violation, but that's all I can read from it :( My hope is that someone used to working with stl will more easily understand it. So, if someone can help me understand the error and what the fix would be in C++, I can fix the corresponding .i file. This is the code snippet the causes the error: -------------- static SCM _wrap_new_paircA1__SWIG_2 (int argc, SCM *argv) { #define FUNC_NAME "new-paircA1" std::pair< int const,A * > *arg1 = 0 ; std::pair< int const,A * > temp1 ; std::pair< int const,A * > *m1 ; SCM gswig_result; SWIGUNUSED int gswig_list_p = 0; std::pair< int const,A * > *result = 0 ; { if (scm_is_pair(argv[0])) { int const* x; A ** y; SCM first, second; first = SCM_CAR(argv[0]); second = SCM_CDR(argv[0]); x = (int const*) SWIG_MustGetPtr(first,SWIGTYPE_p_int,1, 0); y = (A **) SWIG_MustGetPtr(second,SWIGTYPE_p_p_A,1, 0); temp1 = std::make_pair(*x,*y); arg1 = &temp1; } else { arg1 = (std::pair< int const,A * > *) SWIG_MustGetPtr(argv[0],SWIGTYPE_p_std__pairT_int_const_A_p_t,1, 0); } } result = (std::pair< int const,A * > *)new std::pair< int const,A * >((std::pair< int const,A * > const &)*arg1); { gswig_result = SWIG_NewPointerObj (result, SWIGTYPE_p_std__pairT_int_const_A_p_t, 1); } return gswig_result; #undef FUNC_NAME } ------------ And this is the error thrown by g++: ------------- In file included from /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/bits/stl_algobase.h:65:0, from /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/bits/char_traits.h:41, from /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/string:42, from /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/stdexcept:40, from li_std_map_wrap.cxx:1334: /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/bits/stl_pair.h: In member function ‘std::pair<const int, A*>& std::pair<const int, A*>::operator=(const std::pair<const int, A*>&)’: /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/bits/stl_pair.h:88:12: error: non-static const member ‘const int std::pair<const int, A*>::first’, can’t use default assignment operator li_std_map_wrap.cxx: In function ‘scm_unused_struct* _wrap_new_paircA1__SWIG_2(int, scm_unused_struct**)’: li_std_map_wrap.cxx:7429:35: note: synthesized method ‘std::pair<const int, A*>& std::pair<const int, A*>::operator=(const std::pair<const int, A*>&)’ first required here In file included from /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/bits/stl_algobase.h:65:0, from /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/bits/char_traits.h:41, from /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/string:42, from /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/stdexcept:40, from li_std_map_wrap.cxx:1334: /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/bits/stl_pair.h: In member function ‘std::pair<const int, const A*>& std::pair<const int, const A*>::operator=(const std::pair<const int, const A*>&)’: /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/bits/stl_pair.h:88:12: error: non-static const member ‘const int std::pair<const int, const A*>::first’, can’t use default assignment operator li_std_map_wrap.cxx: In function ‘scm_unused_struct* _wrap_new_paircA2__SWIG_2(int, scm_unused_struct**)’: li_std_map_wrap.cxx:7666:35: note: synthesized method ‘std::pair<const int, const A*>& std::pair<const int, const A*>::operator=(const std::pair<const int, const A*>&)’ first required here In file included from /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/bits/stl_algobase.h:65:0, from /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/bits/char_traits.h:41, from /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/string:42, from /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/stdexcept:40, from li_std_map_wrap.cxx:1334: /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/bits/stl_pair.h: In member function ‘std::pair<int, const std::pair<int, A*> >& std::pair<int, const std::pair<int, A*> >::operator=(const std::pair<int, const std::pair<int, A*> >&)’: /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/bits/stl_pair.h:88:12: error: non-static const member ‘const std::pair<int, A*> std::pair<int, const std::pair<int, A*> >::second’, can’t use default assignment operator li_std_map_wrap.cxx: In function ‘scm_unused_struct* _wrap_new_pairiiAc__SWIG_2(int, scm_unused_struct**)’: li_std_map_wrap.cxx:8208:52: note: synthesized method ‘std::pair<int, const std::pair<int, A*> >& std::pair<int, const std::pair<int, A*> >::operator=(const std::pair<int, const std::pair<int, A*> >&)’ first required here make[2]: *** [libli_std_map.so] Error 1 ----------- Thank you, Geert |
From: William S F. <ws...@fu...> - 2013-04-23 18:11:49
Attachments:
guile-pair.patch
|
On 19/04/13 18:41, Geert Janssens wrote: > Hi, > > I'm working through the failing testcases for guile. One testcase fails > with a compilation error in some generated code based on std_map and > std_pair. I know exactly where this generated code comes from, but my > C++ knowledge is insufficient to understand the error, let alone remedy > it. I gather some kind of const violation, but that's all I can read > from it :( My hope is that someone used to working with stl will more > easily understand it. > > So, if someone can help me understand the error and what the fix would > be in C++, I can fix the corresponding .i file. > > This is the code snippet the causes the error: > <snip> > > /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/bits/stl_pair.h:88:12: > error: non-static const member ‘const std::pair<int, A*> std::pair<int, > const std::pair<int, A*> >::second’, can’t use default assignment operator This is the main problem - it is saying there is no default assignment operator (because one of the members in pair is const). The solution is to avoid assignment. I've attached a patch to do this plus some error message corrections. William |
From: Geert J. <in...@ko...> - 2013-04-23 19:33:05
|
On Tuesday 23 April 2013 19:11:29 William S Fulton wrote: > On 19/04/13 18:41, Geert Janssens wrote: > > Hi, > > > > I'm working through the failing testcases for guile. One testcase fails > > with a compilation error in some generated code based on std_map and > > std_pair. I know exactly where this generated code comes from, but my > > C++ knowledge is insufficient to understand the error, let alone remedy > > it. I gather some kind of const violation, but that's all I can read > > from it :( My hope is that someone used to working with stl will more > > easily understand it. > > > > So, if someone can help me understand the error and what the fix would > > be in C++, I can fix the corresponding .i file. > > > This is the code snippet the causes the error: > <snip> > > > /usr/lib/gcc/i686-redhat-linux/4.7.2/../../../../include/c++/4.7.2/bits/st > > l_pair.h:88:12: error: non-static const member ‘const std::pair<int, A*> > > std::pair<int, const std::pair<int, A*> >::second’, can’t use default > > assignment operator > This is the main problem - it is saying there is no default assignment > operator (because one of the members in pair is const). The solution is > to avoid assignment. I've attached a patch to do this plus some error > message corrections. > > William I have tested your patch on both my platforms (Fedora/Guile 1.8 and Ubuntu/Guile 2.0) and it fixes the testcase twice. Great and thank you for helping out ! I notice that you do more than just strictly fixing the invalid const assignment. I would guess those are small changes that come from experience and best-practice programming. Would you mind shortly describing why you did the following changes, so (and perhaps others on the list) can learn from them ? - %typemap(in) pair<T,U> (std::pair<T,U>* m) { + %typemap(in) pair<T,U> %{ Here I notice you choose to change from {} delimiters to %{ %}. I read in the docs this means that the code block will not be parsed by the swig preprocessor. The code seemed to work ok before as well (other than the const assignment bug). So what made you change it ? You also remove the (std::pair<T,U>* m) part. I don't know why it was there, or why you removed it. - } - %typemap(in) const pair<T,U>& (std::pair<T,U> temp, - std::pair<T,U>* m), - const pair<T,U>* (std::pair<T,U> temp, - std::pair<T,U>* m) { + %} + %typemap(in) const pair<T,U>& (std::pair<T,U> *temp = 0), + const pair<T,U>* (std::pair<T,U> *temp = 0) %{ Here you do something similar. I'm mostly interested in why you changed (std::pair<T,U> temp, std::pair<T,U>* m) into (std::pair<T,U> *temp = 0) ? %typemap(argout) const pair<T,U>&, const pair<T,U>* %{ delete temp$argnum; %} I gather this is added because you allocated memory using new, which has to be freed. While reading the docs to get to understand this, I also found the "freearg" typemap. With my limited knowledge I interpreted that typemap for this use case. Can you clarify my confusion here ? I hope you won't find my questions too basic... I really feel I have much to learn still. Oh, and lastly, what do I do with your patch ? Do I add it to my branch (and then how do I attribute you for it) ? Or will you apply it yourself after my patches have been merged in ? Geert |
From: William S F. <ws...@fu...> - 2013-04-23 22:14:56
|
On 23/04/13 20:32, Geert Janssens wrote: > I have tested your patch on both my platforms (Fedora/Guile 1.8 and > Ubuntu/Guile 2.0) and it fixes the testcase twice. Great and thank you > for helping out ! > > I notice that you do more than just strictly fixing the invalid const > assignment. I would guess those are small changes that come from > experience and best-practice programming. Well, the compiler was telling me that it cannot assign if one of the types is const and given that some users might have a std::pair like this, a workaround was needed. > > Would you mind shortly describing why you did the following changes, so > (and perhaps others on the list) can learn from them ? > > - %typemap(in) pair<T,U> (std::pair<T,U>* m) { > > + %typemap(in) pair<T,U> %{ > > Here I notice you choose to change from {} delimiters to %{ %}. I read > in the docs this means that the code block will not be parsed by the > swig preprocessor. The code seemed to work ok before as well (other than > the const assignment bug). So what made you change it ? When using {}, these brackets are inserted into the generated code as two additional and unnecessary lines, so I removed them just to make the code look a bit better. > > You also remove the (std::pair<T,U>* m) part. I don't know why it was > there, or why you removed it. > I don't know why it was there either, it looks like some copy/paste left over and wasn't being used. > (std::pair<T,U> temp, std::pair<T,U>* m) into (std::pair<T,U> *temp = 0) ? > These values in brackets are additional variables generated into the wrapper function. As mentioned, 'm' wasn't being used so I got rid of it and I changed temp to be a pointer initialised to zero instead of a variable stored by value which then needs assigning to. This is the workaround - instead of creating a pair and then assigning to it on the stack, which gives a compiler error, just create it once on the heap. A smart pointer would have been better than a pointer, but the STL doesn't have a smart pointer that can be consistently used across old and new versions of STL/C++. I could probably have used SwigValueWrapper though. Note that temp is only used if the type is a guile pair (as determined by scm_is_pair), rather than when the type is a std::pair wrapper. It would have been nicer to create and initialise temp on the stack, but the if statement was too complex to do this in the scope outside of the if block (the scope of temp would need to last until after the wrapped constructor call was made). > %typemap(argout) const pair<T,U>&, const pair<T,U>* %{ delete > temp$argnum; %} > > I gather this is added because you allocated memory using new, which has > to be freed. While reading the docs to get to understand this, I also > found the "freearg" typemap. With my limited knowledge I interpreted > that typemap for this use case. Can you clarify my confusion here ? > I'm glad you looked at this closely as "freearg" is more strictly correct, even though "argout" works. Could you correct this please and just double check the delete is still being generated in the same way. > I hope you won't find my questions too basic... I really feel I have > much to learn still. > > Oh, and lastly, what do I do with your patch ? Do I add it to my branch > (and then how do I attribute you for it) ? Or will you apply it yourself > after my patches have been merged in ? Use 'git am' to commit the patch to your branch. You should save the email and give the location of this file to 'git am'. That will then include the email text in the commit message, which might be interesting to keep, plus have me as the author and you as the committer, which works well as I don't have write access to your branch. 'git am' is cool. William |
From: Geert J. <in...@ko...> - 2013-04-24 14:14:11
|
Thanks for the detailed explanation. That clears up a lot for me. I have replaced argout with freearg and verified this still works. Geert |