[BUG] introducing adding as_const() to for-loop make it impossible to use rvalue container

[filipsajdak]

de630b9 introduce the call to std::as_cast when for loop uses in passing style. That makes it fail when dealing with rvalue containers.

The following code:

main: () -> int = {
    for returns_a_copy() do :(e) = {
        std::cout << e << std::endl;
    }
}

returns_a_copy: () -> std::vector<int> = {
    return std::vector<int>(10);
}

Generates (skipping boilerplate):

[[nodiscard]] auto main() -> int{
    for ( auto&& cpp2_range = std::as_const(returns_a_copy());  auto const& e : cpp2_range ) {
        std::cout << e << std::endl;
    }
}

[[nodiscard]] auto returns_a_copy() -> std::vector<int>{
    return std::vector<int>(10); 
}

And fails to compile with an error:

../tests/bug_as_const.cpp2:2:31: error: call to deleted function 'as_const'
    for ( auto&& cpp2_range = std::as_const(returns_a_copy());  auto const& e : cpp2_range ) {
                              ^~~~~~~~~~~~~
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/__utility/as_const.h:31:6: note: candidate function [with _Tp = std::vector<int>] has been explicitly deleted
void as_const(const _Tp&&) = delete;
     ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/__utility/as_const.h:28:51: note: candidate function [with _Tp = std::vector<int>] not viable: expects an lvalue for 1st argument
_LIBCPP_NODISCARD_EXT constexpr add_const_t<_Tp>& as_const(_Tp& __t) noexcept { return __t; }
                                                  ^
../tests/bug_as_const.cpp2:2:15: error: cannot form a reference to 'void'
    for ( auto&& cpp2_range = std::as_const(returns_a_copy());  auto const& e : cpp2_range ) {
              ^
2 errors generated.

The issue is that the container is an rvalue, and std::as_const() has explicitly deleted overload for rvalue.

The quick workaround is to write a helper function that handles rvalue by forwarding it or otherwise returns results of std::as_const.

[filipsajdak]

I have tried

main: () -> int = {
    v : std::vector = (1,2,3);
    for v do :(e) = {
        std::cout << e << std::endl;
    }
}

Expecting that there will be a call to std::move() on v as it is the last use of v, but apparently, it does not treat it as a last use of v, or something is suppressing adding of std::move() to v variable.

cppfront generates:

[[nodiscard]] auto main() -> int{
    std::vector v {1, 2, 3}; 
    for ( auto&& cpp2_range = std::as_const(v);  auto const& e : cpp2_range ) {
        std::cout << e << std::endl;
    }
}

and that, of course, compiles and executes fine.

[filipsajdak]

I have played a little bit with the helper function the best I can get in a couple of minutes was: https://godbolt.org/z/dMzjovM19

[filipsajdak]

@hsutter do you know why the following code:

namespace cpp2 {
    template <typename T>
    [[nodiscard]] auto ensure_const_loop(T&& x) noexcept -> decltype(auto) {
        if constexpr ( std::is_rvalue_reference_v<decltype(x)> ) {
            return std::forward<T>(x);
        } else {
            return std::as_const(x);
        }
    }
}

ends up with destroying temporary container that was passed to the function? My assumption was that forwarding temporary should end up with temporary on the return of the function. Unfortunately it is not a case - check here: https://godbolt.org/z/Tz1nh95qc

[gregmarr]

Expecting that there will be a call to std::move() on v as it is the last use of v, but apparently, it does not treat it as a last use of v, or something is suppressing adding of std::move() to v variable.

I would not expect there to be any benefit to wrapping the target of a range based for loop in std::move(). In fact, I would expect that to cause things to fail, such as if the container had begin() && = delete to prevent calling begin() on a temporary so it wouldn't return a dangling iterator.

[switch-blade-stuff]

I agree, moving a container for use within a loop does not make much sense, since the container isn't moved anywhere, only it's elements are used.

Besides, a for loop either modifies a container or accesses it's elements (or both). Modifying a last-use rvalue does not make sense, and you would not need an rvalue for const-access either, and since a loop is part of tha parent function, we are not passing the container outside the current context either.

Essentially, you wouldn't do something like this:

s : std::string = "ab_";
std::move(s)[2] = 'c'; // Modifying a last-use rvalue??

So why would you move it into a loop?

[filipsajdak]

@switch-blade-stuff the current implementation of cppfront compiles the following code

main: () -> int = {
    s : std::string = "testing";
    s[0] = 'a';
}

to (skipping boilerplate)

[[nodiscard]] auto main() -> int{
    std::string s {"testing"}; 
    cpp2::assert_in_bounds(std::move(s), 0) = 'a';
}

So, it calls std::move on the last call of s.

By saying, Expecting that there will be a call to std::move(), I was referring to the current behavior of cppfront that moves the variable on the last call. I have fallen into that trap many times already (see the example here: #231).

I report that behavior to @hsutter just to let him know that, in that case, it behaves differently than I see it usually does.

[gregmarr]

Hmm, I would say that both of those are things that make no sense. Maybe the cpp2 helper functions like this should be excluded from move-on-last-use analysis.

I was thinking "For that matter, there's no reason to do a move into a function that only takes by &," and then I looked at cpp2::assert_in_bounds and found that it does indeed use auto&& so that thought doesn't apply.

[jcanizales]

I don't know the details about how for : ranges are implemented, so maybe it's obvious, but wouldn't emitting this code instead fix the original issue?

[[nodiscard]] auto main() -> int{
    for ( auto&& cpp2_range = returns_a_copy();  auto const& e : std::as_const(cpp2_range) ) {
        std::cout << e << std::endl;
    }
}```

[jcanizales]

Oh nevermind, fixed as I was typing haha

[hsutter]

Thanks! Now fixed... on second thought using as_const was needless, I should just declare auto const& cpp2_range.

@hsutter do you know why the following code: ... ends up with destroying temporary container that was passed to the function?

Looks like that long-standing lifetime bug that P2644 fixes -- we finally adopted that fix two months ago in Kona, for C++23. Which BTW is another reason not to use the original as_const call, because it exercises that very bug in pre-C++23 compilers (which, today, means all of them :) ).

[gregmarr]

@hsutter Should we pull out the #234 (comment) related discussion into a separate issue?

[hsutter]

@gregmarr Do you mean including a use that is a range-for range as a last use?

[filipsajdak]

I wonder if it is accidental that there is no move from last use of the container (when last use is in for-loop) or is it by design.

[gregmarr]

@hsutter That, and also that this:

s[0] = 'a';

compiles to this:

cpp2::assert_in_bounds(std::move(s), 0) = 'a';

Maybe it's fine behavior-wise, but it definitely looks weird.