Add a warning for lossy integer division of constants

[RedBeard0531]

I was kinda surprised that return 1 / 2; produces no warnings, even with -Weverything. That feels like a natural way to write one half, for someone who isn't thinking of integers and truncating division. And since it seems like you would never intentionally write that[0], it should have a warning. I know there are already some warnings where arithmetic with constants doesn't evaluate to the numeric result (eg due to overflow) and this seems to fit right in there. It could even become a single warning about any time that numeric results aren't preserved (excluding bitwise operators and common patterns like -1ull to get a bunch of 1 bits).

[0] Ignoring obfuscated code contests...

[Explorer09]

What if I do this:

(~0UL / (1UL << 20))

Will I get a warning?

[firewave]

It is being reported by clang-tidy:

<source>:2:12: warning: result of integer division used in a floating point context; possible loss of precision [bugprone-integer-division]
    2 |     return 1 / 2;
      |            ^

https://godbolt.org/z/Wq9xnbE4K

[RedBeard0531]

It is being reported by clang-tidy

That is a different issue than the one I was interested in and an artifact of how I typed the mini-repro into godbolt. https://godbolt.org/z/31Eh13TK9 void seconds(int); seconds(1/2); is closer to the original which is still something I think clang should warn on since there is no good reason to write that rather than seconds(0) if that is really what you want.

For full context, I was initially looking into this because someone was complaining that seconds(1) / 2 was seconds(0) rather than (eg) milliseconds(500) (where both are types like those in std::chrono). I pointed out that this was defined to be (essentially) equivalent to seconds(1 / 2) and was a bit surprised to not see a warning when I typed that. I can solve this for our type by adding an __attribute__((diagnose_if(...))) on our operator/ to warn when integer division of constants isn't precise. But it seemed like having that warning in general for integers would be nice.

[RedBeard0531]

(~0UL / (1UL << 20))

Will I get a warning?

I dunno. That feels like an obfuscated way to write whatever you are trying to express. I guess it is max ULONG bytes in mebibytes? But since you are using truncation it isn't exactly that value, nor even the rounded version (which would be 1 MiB higher), so it also seems reasonable to warn. If you really want that exact number, why not write it as ~0UL >> 20 which seems a bit clearer anyway?

[Explorer09]

@RedBeard0531

(~0UL / (1UL << 20))
Will I get a warning?

I dunno. That feels like an obfuscated way to write whatever you are trying to express. I guess it is max ULONG bytes in mebibytes? But since you are using truncation it isn't exactly that value, nor even the rounded version (which would be 1 MiB higher), so it also seems reasonable to warn. If you really want that exact number, why not write it as ~0UL >> 20 which seems a bit clearer anyway?

My assumption is that I have a bit mask defined as bit 20 in a macro:

#define FLAG_SUPPORT_FOO (1 << 20)
#define FLAG_SUPPORT_BAR (1 << 21)
#define FLAG_SUPPORT_BAZ (0x7 << 23)

And for some reason I have to shift the bit from bit 20 to bit 0, and back to bit 20 afterward. For this case it's easier to use division by the macro constant rather than right shift. This is one of example of how your proposed warning can over-warn things.

Note I'm not against implementing the warning in general, but I suggest it should be off by default. And also allow exceptions such as the all-ones case (~0L) I mentioned above.

[firewave]

I think the "problem" is that you are doing an "integer division" in an "integer context" (borrowing the language of the clang-tidy warning) - so everything works as intended. Unless there is any indication that you actually want a floating-point value (either by storing it as such or using such in the calculation) this is expected behavior. If you would warn about this it would probably cause tons of false positives.

[Explorer09]

My concern is that there are a lot of valid uses of truncate division in integer contexts, and when the truncate division is intended by the developers, there's no easy way to suppress the warning once you add it.

For integer division in floating point context, at least you can indicate what you mean ((double)(int)(1 / 2) vs. ((double)1 / 2)) so that the compiler won't complain. If you extend the warning to integer context, you need to tell me how to get rid of the warning when I want to do truncate division intentionally.