for_each causes funny behavior in phoenix V3 expressions

[Chromatix]

Using phoenix::for_each in a Phoenix expression produces a strange behavior, which seems to be reverse execution of the comma-separated expressions. This does NOT happen when you include Phoenix V2 from the spirit directory.

Sample Code: (Full code attached)

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

This originally showed up for me when using phoenix expressions in spirit semantic actions, discussed here: ​http://boost.2283326.n4.nabble.com/Phoenix-V3-for-each-or-lambda-seem-to-be-ruining-qi-semantic-actions-tp4659691.html

[John Fletcher <J.P.Fletcher@…>]

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

[John Fletcher <J.P.Fletcher@…>]

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

[Chromatix <mhazadmanesh2009@…>]

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.

[John Fletcher <J.P.Fletcher@…>]

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.

[John Fletcher <J.P.Fletcher@…>]

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

[John Fletcher <J.P.Fletcher@…>]

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]

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

[Joel de Guzman]

that last comment is from me, btw.

[John Fletcher <J.P.Fletcher@…>]

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.

[John Fletcher <J.P.Fletcher@…>]

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.

[Chromatix <mhazadmanesh2009@…>]

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 >

[John Fletcher <J.P.Fletcher@…>]

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.

[John Fletcher <J.P.Fletcher@…>]

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

[Eric Niebler]

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.

[John Fletcher <J.P.Fletcher@…>]

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.