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] z: u8 = x + y; does not compile #1318

Open
MatthieuHernandez opened this issue Oct 13, 2024 · 14 comments
Open

[BUG] z: u8 = x + y; does not compile #1318

MatthieuHernandez opened this issue Oct 13, 2024 · 14 comments
Labels
bug Something isn't working

Comments

@MatthieuHernandez
Copy link
Contributor

The following code does not compile.

main.cpp2

main: () = {
    x: u8 = 1;
    y: u8 = 2;  
    z: u8 = x + y;
    std::cout << z << std::endl;
}

main.cpp1 generated

auto main() -> int{
    cpp2::u8 x {1}; 
    cpp2::u8 y {2}; 
    cpp2::u8 z {cpp2::move(x) + cpp2::move(y)}; 
    std::cout << cpp2::move(z) << std::endl;
}

Compiler error

main.cpp2: In function 'int main()':
main.cpp2:4:31: error: narrowing conversion of '(((int)cpp2::move<unsigned char&>(x)) + ((int)cpp2::move<unsigned char&>(y)))' from 'int' to 'cpp2::u8' {aka 'unsigned char'} [-Wnarrowing]

Example
https://cpp2.godbolt.org/z/r6r9d8KMP

@MatthieuHernandez MatthieuHernandez added the bug Something isn't working label Oct 13, 2024
@JohelEGP
Copy link
Contributor

That's integral promotion.

@MatthieuHernandez
Copy link
Contributor Author

Yes indeed, but is it a bug?
It's really annoying and I can't even write this to try to correct the problem: z:= (x + y) as u8;

@JohelEGP
Copy link
Contributor

See Type/value queries and casts.
In particular, you can't use the safe as to narrow.
You must use unchecked_narrow<To>(from).

@MaxSagebaum
Copy link
Contributor

Even if it is integral promotion, the C++1 version works without any problems. https://godbolt.org/z/PEfeaPvn1

#include <iostream>
#include <cstdint>

int main() {
    std::uint8_t x = 1;
    std::uint8_t y = 2;
    std::uint8_t z = x + y;

    std::cout << z << std::endl;
}

So even if it is not a bug, it is a strong inconvenience. Maybe cpp2::move needs to be extended to prevent this.

@JohelEGP
Copy link
Contributor

It's because the lowered Cpp1 uses list-initialization, which prevents narrowing.

@MaxSagebaum
Copy link
Contributor

Ok, I had a closer look. List-initialization for z still compiles https://godbolt.org/z/YY6fhbbe7. But gives the warning.
If we remove the move in the cpp2 example we get the ``same'' warning, which is an error in the compiler settings chosen: https://cpp2.godbolt.org/z/do8Ws4ePx

main: () = {
    x: u8 = 1;
    y: u8 = 2;  
    z: u8 = x + y;
    std::cout << x << " + " << y << " = " << z << std::endl;
}

Error:

main.cpp2:4:19: error: narrowing conversion of '(((int)x) + ((int)y))' from 'int' to 'cpp2::u8' {aka 'unsigned char'} [-Wnarrowing]

So it is the integral promotion you suggested. This seems to be one of those corner cases where the language behaves quite weird. If this would be a generalized rule then also

int main() {
    int x = 1;
    int y = 2;
    int z = x + y;

    std::cout << z << std::endl;
}

should give the same warning.

@gregmarr
Copy link
Contributor

This makes me wonder if "move from last use" should ignore fundamental types, just to avoid the unnecessary complexity in the generated code.

@JohelEGP
Copy link
Contributor

I also think cpp2::move should also go.
Now that compilers are moving towards treating std::move like a built-in for compilation speed,
cpp2::move is a step backwards from that.

@DyXel
Copy link
Contributor

DyXel commented Oct 14, 2024

How would you constrain it in that case though? cpp2::move does certain things differently, take a look at #1030.

@gregmarr
Copy link
Contributor

cpp2::move was added here 8f4e855#diff-9e3e6d623803ce8a26a44e27a5824cf938eae42d04829e2a41b80bd40a43a246 while fixing #968 and #999. It originally did both move and forward. The forward was intentionally removed here de3f54f so there may not be any reason for this to still exist.

@gregmarr
Copy link
Contributor

gregmarr commented Oct 14, 2024

@DyXel It doesn't really do things differently any more. @hsutter couldn't remember why it did things differently, so maybe the need was removed in another way, or maybe it's just been broken again.

It's now just this:

template <typename T>
constexpr auto move(T&& t) -> decltype(auto) {
    return std::move(t);
}

@JohelEGP
Copy link
Contributor

See the linked commit's comments: de3f54f#comments.

@DyXel
Copy link
Contributor

DyXel commented Oct 14, 2024

Huh, that's interesting. I thought the original problem was that some objects that were supposed to not be moved were being moved. Like the original example in #1002 . Its quite inconvenient if the common pattern of locking a mutex would require you to discard the mutex later.

@gregmarr
Copy link
Contributor

@JohelEGP Thanks, I didn't scroll to the bottom. :)

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

5 participants