[PATCH] D38101: [Sema] Diagnose tautological comparison with type's min/max values

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: lib/Sema/SemaChecking.cpp:8719
+  // Type limit values are handled later by CheckTautologicalComparison().
+  if (IsTypeLimit(S, Other, Constant, ConstValue) && (!OtherIsBooleanType))
     return;
----------------
lebedev.ri wrote:
> rsmith wrote:
> > Is this necessary? (Aren't the type limit values always within the range of the type in question?)
> > 
> > Can we avoid evaluating `Constant` a extra time here? (We already have its value in `Value`.)
> Uhm, good question :)
> If i remove this, `check-clang-sema` and `check-clang-semacxx` still pass.
> I agree that it does not make much sense. Initially it was only checking for `Value == 0`.
> Git suggests that initially this branch was added by @rtrieu, maybe can help.
[[ https://github.com/llvm-mirror/clang/commit/526e627d2bd7e8cbf630526d315c90864898d9ff#diff-93faf32157a807b1b7953f3747db08b6R4332 | The most original version of this code ]]
After some though i think the initial check `Value == 0` was simply to quickly bail out
out of `DiagnoseOutOfRangeComparison()`, and not waste any time for the obvious case
(`0`), which can't be out-of-range, ever. So i think the right behaviour could be:
1. Either simply restore the original check:
```
// 0 values are handled later by CheckTautologicalComparison().
if ((Value == 0) && (!OtherIsBooleanType))
  return;
```
And add a comment there about it
2. Or, drop it completely
3. Or, perhaps refactor `CheckTautologicalComparison()`, and more or less call it from
     function `AnalyzeComparison()`, before calling `DiagnoseOutOfRangeComparison()`,
     thus completely avoiding the need to re-evaluate `Constant` there later on,
     and simplify the logic in the process.

I personally think the `3.` *might* be best.
WDYT?


Repository:
  rL LLVM

https://reviews.llvm.org/D38101