Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Move on definitive last use in fold expressions #1279

Open
Velocirobtor opened this issue Sep 16, 2024 · 5 comments
Open

[BUG] Move on definitive last use in fold expressions #1279

Velocirobtor opened this issue Sep 16, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@Velocirobtor
Copy link

Describe the bug
If the definitive last use of a variable is inside a fold expression, cppfront will still emit cpp2::move(<variable>) even though this will most likely lead to multiple moves from <variable>.

To Reproduce
Compile the following cpp2 code:

foo: (forward item: _) = {
    std::print(" {}", std::is_rvalue_reference_v<decltype(item)>);
}
bar_fold: <Is...: std::size_t>() = {
    test := 42;
    ((Is, foo(test)), ...);
}
int main() {
    bar_fold<0, 1>();
}

This will generate this cpp1 code (omitting forward declarations):

auto foo(auto&& item) -> void{
    std::print(" {}", std::is_rvalue_reference_v<decltype(CPP2_FORWARD(item))>);
}
template<std::size_t ...Is> auto bar_fold() -> void{
    auto test {42}; 
    ((Is, foo(cpp2::move(test))), ...);
}
int main() {
    bar_fold<0, 1>();
}

which prints " true true"

Expected Behavior
I think one of the following should happen:

  • It behaves like an "equivalent" for loop -> it prints " false false"
  • It behaves like an "equivalent" manually unrolled implementation -> it prints " false true"

The first would just require not emitting cpp2::move in fold expressions. The latter could be achieved by generating code somewhere along the lines of:

namespace cpp2 {
    template<bool Condition>
    constexpr decltype(auto) move_if(auto&& val) {
        if constexpr(Condition) {
            return move(val);
        } else {
            return val;
        }
    }
}
template<std::size_t... Is>
void bar() {
    auto test{ 42 };
    [&]<std::size_t... GeneratedIndices>(std::index_sequence<GeneratedIndices...>) -> decltype(auto) {
        return ((Is, foo(cpp2::move_if<GeneratedIndices + 1 == sizeof...(Is)>(test))), ...);
    }(std::make_index_sequence<sizeof...(Is)>{});
}

Link to Godbolt with the complete example: https://cpp2.godbolt.org/z/fcjWPfhsh

P.S. How can I silence the warning left operand of comma operator has no effect using cpp2 Syntax? I tried static_cast<void>(Is) which lead to cppfront suggesting Is as void, which didn't work.

@Velocirobtor Velocirobtor added the bug Something isn't working label Sep 16, 2024
@JohelEGP
Copy link
Contributor

JohelEGP commented Oct 4, 2024

P.S. How can I silence the warning left operand of comma operator has no effect using cpp2 Syntax? I tried static_cast<void>(Is) which lead to cppfront suggesting Is as void, which didn't work.

You discard it with _ = Is (https://cpp2.godbolt.org/z/foP3fMzx8), see _ — the "don't care" wildcard, including explicit discard.

@Velocirobtor
Copy link
Author

You discard it with _ = Is (https://cpp2.godbolt.org/z/foP3fMzx8), see _ — the "don't care" wildcard, including explicit discard.

Awesome, thank you! For some reason I convinced myself that I couldn't use that in an expression and just as a standalone statement and didn't even try to use it... Glad to see it was just a user error on my part :)

@JohelEGP
Copy link
Contributor

JohelEGP commented Oct 7, 2024

I tried static_cast<void>(Is) which lead to cppfront suggesting Is as void, which didn't work.

Looks like there's an opportunity for improving the diagnostic here.
The general diagnostic is for preferring as to cast.
For the void case, suggesting _ = Is is more appropriate.

@Velocirobtor
Copy link
Author

Velocirobtor commented Oct 7, 2024

Yeah, that would have definitely helped me and I'd guess anyone else starting out with cpp2 coming from a cpp background. Though I'm not sure if the target type is available at the point of emitting the diagnostic, as the actual message was:
error: 'static_cast<T>(val)' is not supported in Cpp2 - use 'val as T' ... (notice the use of T and not void)

hsutter added a commit that referenced this issue Oct 7, 2024
…'as void'

For replacing 'unsafe_*': Thanks to Redditor @tialaramex for their suggestion here, presuasively citing Rust's naming style (and thanks to the Rust designers too for the inspiration):
https://www.reddit.com/r/cpp/comments/1euacbu/cpp2_is_looking_absolutely_great_will_convert/liy6g8r/

For replacing 'as void': Thanks to @Velocirobtor and @JohelEGP for their suggestions on improving this diagnostic in the #1279 comment thread.
@hsutter
Copy link
Owner

hsutter commented Oct 7, 2024

Looks like there's an opportunity for improving the diagnostic here.

Thanks! This part is done: 07b0eed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants