GNOME Bugzilla – Bug 342911
Only last lambda expression from list separated by comma is executed
Last modified: 2013-03-04 15:05:40 UTC
Please describe the problem: I tried to connect signal<void, int, int> following lambda expression: hide_return((var(a)=_1, var(b)=_2)) where a and b are integer variables. Only the second assignment is performed when signal is emmitted. Steps to reproduce: 1. 2. 3. Actual results: The value of a remains unchanged and a new value is assigned to b Expected results: The new values should be asigned to both variables a and b Does this happen every time? Yes Other information:
Created attachment 66183 [details] Test example for a bug Command line for compiling the example: g++ `pkg-config --cflags sigc++-2.0` bug_20060524.cpp `pkg-config --libs sigc++-2.0` -lboost_signals -o bug_20060524 Boost (http://www.boost.org) is used for comparisson. With it I'm getting the expected behaviour.
Created attachment 66193 [details] Updated test example (compares also result of comma operator)
Created attachment 66194 [details] [review] Patch that fixes the problem for me Unfortunatelly I cannot use M4 macro LAMBDA_OPERATOR as I'm getting about left side of comma operator having no effect. So I had to add cast to void. Perhaps we don't need to support comma operator when one side is not a lambda type. Test environment: Fedora Core 5, gcc version 4.1.0 20060304 (Red Hat 4.1.0-3).
I don't really feel qualified to review this because I don't use this libsigc++ feature. But I don't like that we have ignored this patch.
[Resetting QA Contact and Assignee to default - see bug 659799 for more info]
Verified that 1) bug is still there in libsigc++-2.2.10 (tested in Fedora 15 x86_64) 2) attached patch still applies without problems and also FIXES the bug for that version (sources from ftp.gnome.org, not Fedora distribution)
Review of attachment 66194 [details] [review]: ::: sigc++/adaptors/lambda/macros/operator.h.m4 @@ +292,3 @@ struct static_ {}; struct dynamic_ {}; +struct comma {}; Should this be comma_ rather than comma?
I don't think I've ever seen comma operator overloading before. Do we do this already in libsigc++? I would also like a test case to be integrated with the build (during make check) like the existing checks.
It would be nice to add testcase. What could be best place for it? I wanted to put it into test_lambda.cc, but it's not built by 'make check' Maybe it should be enabled and excluded only for targets where it does not build (either by build configuration or using C++ preprocessor defines specific for target)
(In reply to comment #9) > Maybe it should be enabled and excluded only for targets > where it does not build That sounds good. > (either by build configuration or using C++ > preprocessor defines specific for target) A configure test would be best.
(In reply to comment #9) > I wanted to put it into test_lambda.cc, but it's not > built by 'make check' See bug 669128.
Andris, will you update your patch?
As far as I have tested earlier with the latest version I see on ftp.gnome.org (2.2.10) the old patch is still OK (comment 6). Also updated test example can be used with only small modifications in testsuite: - remove boost part - check result of assignments from lambda expression Rebuilt this test example on Fedora 17 x86_64 and got: [andris@ap sigc++]$ g++ -O2 $(pkg-config --cflags sigc++-2.0) -Wall -Wextra comma_test.cpp $(pkg-config --libs sigc++-2.0) -lboost_signals-mt [andris@ap sigc++]$ ./a.out BOOST: a=2 b=3 c=3 libsigc++: a=-1 b=3 c=3 [andris@ap sigc++]$ pkg-config --modversion sigc++-2.0 2.2.10 I have not tested patch with the current development sources though
Created attachment 224847 [details] [review] patch: Fix comma operator in lambda expressions. Here's a patch that includes both the additions to operator.h.m4 (without the preprocessor constant __SIGC_LAMBDA_COMMA_OPERATOR_NAME) and a test case. The test case differs from the one in comment 2, because I wanted it to be more similar to most other tests in test_lambda.cc (without sigc::slot and sigc::signal). (In reply to comment #7) > Should this be comma_ rather than comma? I don't think so. If you look at the whole list of empty structs in operator.h.m4, you will see that most of them have names that do not end with an underscore. Most of the underscores are added because the names would otherwise be equal to a C++ keyword. (At least that's what I think is the main reason.)
Looks OK for me. I myself would prefer using of cppunit or some some similar library for testing instead of having to review test results though
OK. Feel free to push this.
I've pushed the patch in comment 14. (In reply to comment #15) > I myself would prefer using of cppunit or some some similar library for > testing instead of having to review test results though Most of the test cases in libsigc++2/tests are not well suited for their job. They produce output that someone must check manually. They should be quiet when everything is OK and report the result with an exit status. As it is, 'make check' will only detect compile-time errors, but not run-time errors, because usually no one will check the output (I suspect). I have thought of fixing at least some of the test cases. Perhaps I will do it in the future. (No promise!) That's beyond the scope of this bug.
(In reply to comment #17) > I have thought of fixing at least some of the test cases. Perhaps I will do > it in the future. (No promise!) That's beyond the scope of this bug. Done some time ago. See bug 684956.