Boost C++ Libraries: Ticket #9742: for_each causes funny behavior in phoenix V3 expressions

attachment set

Wed, 05 Mar 2014 08:07:20 GMT

attachment → boostphxtest.cpp

Mon, 10 Mar 2014 16:53:20 GMT

Hi. Note I have taken on maintenance of Phoenix but the Trac system does not have that information yet. I have observed this myself and there are some problems with the comma operator in 1.55.0 and earlier. I will try your test against the current development which has some fixes. Some will be in 1.56.0 when it comes out. I had seen similar things myself before I became maintainer. John

Mon, 10 Mar 2014 22:17:51 GMT

I have run your test with my latest develop version and unfortunately the bug is still there. It is compiler dependent. gcc 4.6 and 4.8.2 give bad results and clang 3.4 gives correct on Ubuntu Linux. I do not have a work around for it on the failing compilers. John

Thu, 13 Mar 2014 20:47:48 GMT

Hello.

Sorry I forgot to mention my original code that failed (and the attached file) was tested on MSVC 2010.

Thanks for taking care btw.

Sun, 16 Mar 2014 10:19:11 GMT

With further testing I have shown that the following code does NOT show the error with gcc 4.6 and gcc 4.8.2. Clang 3.4 also works.

#include <sstream>
    std::ostringstream out;
    (
        out << phx::val("("),
        phx::for_each(arg1, phx::lambda[out << arg1]),
        out << phx::val(")")
    )(v);
    std::cout << out.str() << std::endl;

I don't have an explanation for why it doesn't work with std::cout. Would you please test with MSVC and see if this works for that as well.

Sun, 16 Mar 2014 11:37:59 GMT

Sorry, my mistake, the code above still does not work with gcc 4.6 and 4.8.2

Sun, 16 Mar 2014 18:58:13 GMT

I have done some more experiments. The following code, using for_ in place of for_each does work for the compilers I have tested:

    int iii;
    int size = v.size();
    (
        std::cout << phx::val("("),
        phx::for_(phx::ref(iii) = 0, phx::ref(iii) < size, ++phx::ref(iii) )
        [std::cout << arg1[phx::ref(iii)] ],
        std::cout << phx::val(")")
    )(v);

anonymous — Mon, 17 Mar 2014 03:38:07 GMT

that workaround does not fix the problem. have you tried wrapping out in a ref or cref?

Joel de Guzman — Mon, 17 Mar 2014 03:39:05 GMT

that last comment is from me, btw.

Mon, 24 Mar 2014 13:21:02 GMT

I am no nearer to solving this. I have put two new tests onto Phoenix develop - for_test and for_each in the hope of finding the failures. However, the tests pass on all compilers, even gcc 4.6 and 4.8.2 where my own tests fail. I must be missing something. John.

Wed, 02 Apr 2014 19:59:58 GMT

I think I now have a reason for this. The implementation of phoenix::for_each is located in boost/phoenix/stl/algorithm/iteration.hpp. The actual code is this:

            template<class R, class F>
            F const operator()(R& r, F const& fn) const
            {
                return std::for_each(detail::begin_(r), detail::end_(r), fn);
            }

The "C++ Standard Library" N.M.Josuttis p.300 explains the rationale for a return type from for_each. However in this case replacing the code by

            template<class R, class F>
            F const operator()(R& r, F const& fn) const
            {
                std::for_each(detail::begin_(r), detail::end_(r), fn);
            }

and making the return type void to code works. I think the return object is a temporary which is getting discarded and invalidating the return mechanism in the comma operator. I will discuss this with the other maintainers.

Wed, 02 Apr 2014 21:51:01 GMT

Erm, in the mailing list thread whose link is in the initial post, TONGARI J already suggested that changing the return type to void works and yes it does on VC 2010; I didn't know if you saw it and was unsure if I had to bring this up sooner, sorry.

< useless novice thoughts >

After reading the rationale, it seems for_each's return value might be useful in some cases, and removing it may cause inconsistency between the original and lazy version of the for_each. Phoenix V2's for_each does return std::foreach's return value but this problem doesn't exist there.

By the way, does adding another layer of laziness to the return value help? e.g wrapping the functor returned by std::for_each inside another functor class and returning that one from phoenix::for_each, or returning a phoenix::(c)ref of std::for_each's return value, or returning phoenix::bind of std::for_each's return value, or maybe by wrapping std::for_each's return value in some proto tags. Due to lack of knowledge, I wasn't successful in implementing any of the above thoughts and testing them (got massive template errors).

</ useless novice thoughts >

Wed, 02 Apr 2014 22:21:26 GMT

Thank you. I was just reading the mailing list thread and saw it had suggested returning void. I arrived at my conclusions by working with the phoenix example code 'container_actor' and comparing a size_impl functor which works with the for_each code. I have looked into what is happening at the Proto level, and the code in the bracket is passed to a set of comma operators. There is a Proto operation called display_expr() which makes this clear. The ones containing the return object from for_each get wrongly processed, that is if it is (a,b) then b gets processed before a, but only by some compilers and only sometimes. My tests on develop work when I don't expect them to. Changing to the void return removes the problem. I need to explore why this happens with the other developers.

When I have implemented a solution I will post it here. I want to make the return object available for cases where it is needed.

Would you let me know what other cases fail? I think you said that there were problems with my for_ solution but I cannot reproduce those with gcc on Linux.

Thank you.

Wed, 02 Apr 2014 22:36:00 GMT

Can you check out using phoenix::accumulate() which in my testing works O.K.? It does return a value.

Eric Niebler — Wed, 02 Apr 2014 23:30:23 GMT

It has something to do with the phx::lambda. The following does *not* exhibit the problem:

std::vector<int> v{1,2};
boost::function<void(int)> fun = std::cout << arg1;
(
  std::cout << phx::val("("),
  phx::for_each(arg1, fun),
  std::cout << phx::val(")")
)(v);

I'd start by looking at what is special about phx expressions that have lambdas. Some transform is getting applied that is doing the wrong thing for comma operators or binary operators that are left-associative.

Mon, 07 Apr 2014 16:43:57 GMT

This case also does not appear to have the problem:

std::vector<int> v{1,2};
boost::function<void(int)> phxlam = phx::lambda[std::cout << arg1]();
(
  std::cout << phx::val("("),
  phx::for_each(arg1, phxlam) ,
  std::cout << phx::val(")")
)(v);

Note the () after the definition of the lambda. The problem seems to occur when the return from the for_each is a lambda_actor object.