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:
> 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?
Tried implementing `3.`.
It won't work, because `DiagnoseOutOfRangeComparison()` needs the `{L,R}HS`
after `IgnoreParenImpCasts()`, and `CheckTautologicalComparison()` is not ok
with that.
It seems that at most, i could re-use the detection of `RhsConstant`.
So, new options:
1. Either simply restore the original check, and add a comment there about the
logic behind it
2. Or, drop the check completely
3. Or, move the `CheckTautologicalComparison()` call before
`DiagnoseOutOfRangeComparison()`
And if `DiagnoseOutOfRangeComparison()` has already emitted diagnostic,
return.
Much like what `CheckTautologicalComparison()` already does.
So i think `3.` is still the best option :)
(tried implementing it, appears to work)
Repository:
rL LLVM
https://reviews.llvm.org/D38101
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
