This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Move some stuff into -Wtautological-unsigned-enum-zero-compare
ClosedPublic

Authored by lebedev.ri on Sep 8 2017, 9:10 AM.

Details

Summary

As requested by Sam McCall:

Enums (not new I guess). Typical case: if (enum < 0 || enum > MAX)
The warning strongly suggests that the enum < 0 check has no effect
(for enums with nonnegative ranges).
Clang doesn't seem to optimize such checks out though, and they seem
likely to catch bugs in some cases. Yes, only if there's UB elsewhere,
but I assume not optimizing out these checks indicates a deliberate
decision to stay somewhat compatible with a technically-incorrect
mental model.
If this is the case, should we move these to a
-Wtautological-compare-enum subcategory?

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Sep 8 2017, 9:10 AM
lebedev.ri added a subscriber: jroelofs.

Rework as per @jroelofs's suggestion to have just one switch/if cascade that operates on BinaryOperatorKind

I'm not sure it's better than writing the if/elseif/elseif/elseif out explicitly :/

And finish reducing the code by for-range-loop`ing over array + use std::array.

And finish reducing the code by for-range-loop`ing over array + use std::array.

I will need to fix handling of the second edge-case (comparison with max unsigned value or with min/max for signed values), so having the code this way *might* help..

aaron.ballman added inline comments.Sep 12 2017, 8:26 AM
lib/Sema/SemaChecking.cpp
8602 ↗(On Diff #114396)

I don't think this is an improvement over the old code.

Aaaand revert back to dump approach with code duplication as requested :)

(For the second part of the fix in another differential, if the reverted approach with struct turns out to be better, i guess i will use it)

Ping, would be really nice to get this going :)
Would unblock me for working on the second half of the fix for https://bugs.llvm.org/show_bug.cgi?id=34147

aaron.ballman added inline comments.Sep 19 2017, 11:21 AM
lib/Sema/SemaChecking.cpp
8593 ↗(On Diff #114861)

Comments should be complete sentences with capitalization and punctuation (here and elsewhere in the patch).

Also, I can't really understand what this comment is saying.

8596 ↗(On Diff #114861)

Should be Cmp per coding standards.

  1. Correctly added TautologicalUnsignedEnumZeroCompare to TautologicalCompare group (sigh)
  2. Hopefully fixed spelling of the comments and added a bit better comments
  3. Fixed variable names to comply with the coding style.
aaron.ballman added inline comments.Sep 19 2017, 1:12 PM
lib/Sema/SemaChecking.cpp
8605 ↗(On Diff #115877)

I'd drop this comment entirely.

8608 ↗(On Diff #115877)

Instead of doing all of this complex logic, why not structure the code like the original and use '?:' to determine which diagnostic id to pass in?

if (op == BO_LT && isNonBooleanUnsignedValue(LHS) && IsZero(S, RHS)) {
  S.Diag(E->getOperatorLoc(), HasEnumType(LHS) ? diag::warn_lunsigned_enum_always_true_comparison :
diag::warn_lunsigned_always_true_comparison)
    << "< 0" << "false"
    << LHS->getSourceRange() << RHS->getSourceRange();

Simplify CheckTautologicalComparisonWithZero() as per review notes.
Less LOC.

This revision is now accepted and ready to land.Sep 19 2017, 1:55 PM
This revision was automatically updated to reflect the committed changes.
lebedev.ri reopened this revision.Sep 19 2017, 2:45 PM

Reverted due to buildbot failures.
Need to investigate.

This revision is now accepted and ready to land.Sep 19 2017, 2:45 PM
This revision was automatically updated to reflect the committed changes.