[PATCH] D93822: [clang][Sema] Add diagnostics for implicit widening of multiplication result

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: NoQ, Szelethus.
aaron.ballman added a comment.

In D93822#2474475 <https://reviews.llvm.org/D93822#2474475>, @dblaikie wrote:

> In D93822#2474355 <https://reviews.llvm.org/D93822#2474355>, @lebedev.ri wrote:
>
>> I'm open to input here, but it would be good to have this at least in `-Wextra` (`-Weverything` always being a last resort option).
>
> Yeah, not sure how @rsmith, @rtrieu, or @aaron.ballman feel about what the bar for warnings is these days - used to be a bit more hardcore about the "if it can't be on by default it shouldn't be in clang" than we are these days, I think. I'd guess -Wextra might be suitable.

I'd like to see data before making a determination, but my gut instinct is that this diagnostic will be far too chatty to be in Clang (even in `-Wextra`) but that this would be a great flow-sensitive check for the clang static analyzer (I've added some analyzer folks to the thread for their input). Static analyzers frequently implement this functionality because they can more easily notice things that limit the range of values possible for a given object and use that extra information when deciding whether overflow is likely or not. e.g., if the analyzer encounters `if (foo > 10) return;` in a method then it can understand that subsequent uses of `foo` after that statement are bounded such that `foo <= 10`. I think we'll ultimately need this flow sensitivity to reduce the false positive rate to something more useful for math-heavy code bases.

Assuming that the data shows a high false positive rate, if you don't have the appetite to work on the robust check in the static analyzer, another option would be to implement this functionality in clang-tidy (which is allowed to be more chatty than the Clang frontend or the static analyzer in terms of false positives). However, I still think we eventually will want the static analyzer check and so my preference is for that approach (so that we don't wind up needing to carry around two implementations of effectively the same check).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93822/new/

https://reviews.llvm.org/D93822