hsutter / cppfront

A personal experimental C++ Syntax 2 -> Syntax 1 compiler
Other
5.51k stars 244 forks source link

[BUG] `Wshorten-64-to-32` clang warning when using range `0 ..< vector.size()` #1269

Closed bluetarpmedia closed 4 weeks ago

bluetarpmedia commented 2 months ago

Describe the bug Clang produces a Wshorten-64-to-32 warning with a cpp2::range when it is initialised with an integer literal as the first member and an expression producing a size_t/size_type as the second member.

To Reproduce Run cppfront on the following code and then compile with clang with -Wconversion:

main: () = {
    v: std::vector = (11, 22, 33);

    for 0 ..< v.size() do (i) {    // Warning
        v[i]++;
    }
    std::println("v: {}", v);

    for 0uz ..< v.size() do (i) {  // No warning with `uz`
        v[i]++;
    }
    std::println("v: {}", v);
}

The warning is:

warning: implicit conversion loses integer precision: 'size_type' (aka 'unsigned long') to 'const std::type_identity_t<int>' (aka 'const int') [-Wshorten-64-to-32]

Repro on Godbolt

This is because the cpp2::range constructor deduces the type from only the first parameter:

range(
    T const&                       f,
    std::type_identity_t<T> const& l,
    bool                           include_last = false
)
DyXel commented 2 months ago

It should probably deduce to the bigger of the two, if possible.

bluetarpmedia commented 2 months ago

It should probably deduce to the bigger of the two, if possible.

Yes, in my PR I use std::common_type and it appears to do just that.

feature-engineer commented 1 month ago

I'm not so sure that this is a good idea. Sometimes you'd want your index to be a signed integer - if you're performing index arithmetic which might become negative, a use of unsigned index would result in a bug - and if the language is making an implicit conversion - it would be a very hard to figure out bug.

I think that this warning is important, but might be made more friendly to explain when to use unsigned index i.e. 0u ..< v.size() vs 0u ..< v.ssize(). Ususally v.ssize() is the right choice, since when you're using an index, you're probably going to use some arithmetic on it - otherwise iterating over the items directly is more succinct and cleaner.

So my suggestion is to make cppfront error out when one of the terms is signed, while the other is unsigned and make the programmer explicitly choose which is appropriate, while providing a nice error message with an explanation.