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 cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits