[clang-tidy] SuspiciousEnumUsageCheck bugfix

[szepet]

Summary

There is a reported bug on the checker not handling the some APSInt values correctly: https://bugs.llvm.org/show_bug.cgi?id=34400

This patch fixes it, however, it shows a false positive. (Added to the test cases)
I am not sure if it's a checker or AST problem. The AST dump shows the following:

-BinaryOperator 0x1195b98 <col:38, col:43> 'int' '|'
 |-ImplicitCastExpr 0x1195b68 <col:38> 'int' <IntegralCast>
 | `-DeclRefExpr 0x1195b18 <col:38> 'enum j<struct c<int, 0, 0, 0, 0, 0> >::(anonymous at dude.cpp:15:3)' EnumConstant 0x1193b98 'ah' 'enum a<struct f<struct c<int, 0, 0, 0, 0, 0>, struct c<int, 0, -1, 0, 0, -1>, 0> >::(anonymous at dude.cpp:29:3)'
 `-ImplicitCastExpr 0x1195b80 <col:43> 'int' <IntegralCast>
   `-DeclRefExpr 0x1195b40 <col:43> 'enum j<struct c<int, 0, -1, 0, 0, -1> >::(anonymous at dude.cpp:15:3)' EnumConstant 0x1195ad0 'ai' 'enum a<struct f<struct c<int, 0, 0, 0, 0, 0>, struct c<int, 0, -1, 0, 0, -1>, 0> >::(anonymous at dude.cpp:29:3)'

I am not sure if this is right since this belongs to the following code snippet (maybe I am just missing something):

enum { ah = ad::m, ai = ae::m, l = ah | ai };

What do you think?

[aaron.ballman]

clang-tidy/misc/SuspiciousEnumUsageCheck.cpp

46 ↗

(On Diff #114180)

I would test for < 0 rather than a direct equality test (compare functions generally follow the C idioms).

61–62 ↗

(On Diff #114180)

Same here, though I would also get rid of the local variables and just perform the comparison in the return statement.

test/clang-tidy/misc-suspicious-enum-usage.cpp

1 ↗

(On Diff #114180)

I thought we built in C++11 mode by default these days?

122 ↗

(On Diff #114180)

This seems like a lot of complicated code for the test case -- can this be reduced further?

[alexfh]

LG modulo comments. Thank you for the fix!

test/clang-tidy/misc-suspicious-enum-usage.cpp

1 ↗

(On Diff #114180)

I'm not sure, but if we actually do, we could later remove -std=c++11 from the test script and all tests.

92 ↗

(On Diff #114180)

I'd suggest enclosing the test case in a namespace PR34400 {} for clarity and to avoid possible name conflicts.

122 ↗

(On Diff #114180)

This is the best creduce could do. It should be possible to make this much shorter, but I wouldn't spend too much time on that.

[aaron.ballman]

test/clang-tidy/misc-suspicious-enum-usage.cpp

122 ↗

(On Diff #114180)

I'd appreciate the time spent because this is an almost-unintelligible test for anyone reading it -- it's hard to understand what's going on there.

[alexfh]

test/clang-tidy/misc-suspicious-enum-usage.cpp

122 ↗

(On Diff #114180)

namespace PR34400 {

enum { E1 = 0 };
enum { E2 = -1 };

enum { l = E1 | E2 };

}

[alexfh]

test/clang-tidy/misc-suspicious-enum-usage.cpp

122 ↗

(On Diff #114180)

This might not cover both code paths affected by the bug. But it should be easy to construct a case for the second one.

[aaron.ballman]

test/clang-tidy/misc-suspicious-enum-usage.cpp

122 ↗

(On Diff #114180)

It's certainly an easier case to grok, thank you! :-D

[szepet]

Updates based on the comments. Thanks for the reduced test case and the fast review!

Not sure if in these cases am I allowed to commit (since it was accepted) or since it was accepted a lot of comments ago I should wait for another response?

[alexfh]

Still LG. Fine to commit.

Thanks for the fix!

If you find time to construct the test case that triggers both affected code paths (my test seems to only inspect one of them, IIUC), it would be nice to do as a follow up.

clang-tools-extra/trunk/clang-tidy/misc/SuspiciousEnumUsageCheck.cpp

 struct ValueRange {
   llvm::APSInt MinVal;
   llvm::APSInt MaxVal;
 
   ValueRange(const EnumDecl *EnumDec) {
     const auto MinMaxVal = std::minmax_element(
         EnumDec->enumerator_begin(), EnumDec->enumerator_end(),
         [](const EnumConstantDecl *E1, const EnumConstantDecl *E2) {
-          return E1->getInitVal() < E2->getInitVal();
+          return llvm::APSInt::compareValues(E1->getInitVal(),
+                                             E2->getInitVal()) < 0;
         });
     MinVal = MinMaxVal.first->getInitVal();
     MaxVal = MinMaxVal.second->getInitVal();
   }
 };
 
 /// Return the number of EnumConstantDecls in an EnumDecl.
 static int enumLength(const EnumDecl *EnumDec) {
   return std::distance(EnumDec->enumerator_begin(), EnumDec->enumerator_end());
 }
 
 static bool hasDisjointValueRange(const EnumDecl *Enum1,
                                   const EnumDecl *Enum2) {
   ValueRange Range1(Enum1), Range2(Enum2);
-  return (Range1.MaxVal < Range2.MinVal) || (Range2.MaxVal < Range1.MinVal);
+  return llvm::APSInt::compareValues(Range1.MaxVal, Range2.MinVal) < 0 ||
+         llvm::APSInt::compareValues(Range2.MaxVal, Range1.MinVal) < 0;
 }
 
 static bool isNonPowerOf2NorNullLiteral(const EnumConstantDecl *EnumConst) {
   llvm::APSInt Val = EnumConst->getInitVal();
   if (Val.isPowerOf2() || !Val.getBoolValue())
     return false;
   const Expr *InitExpr = EnumConst->getInitExpr();
   if (!InitExpr)

clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-enum-usage.cpp

Days bestDay() {
   return Friday;
 }
 
 int trigger() {
   Empty EmptyVal;
   int emptytest = EmptyVal | B;
   if (bestDay() | A)
     return 1;
-  // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types 
+  // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: enum values are from different enum types
   if (I | Y)
     return 1;
   // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: enum values are from different enum types
 }
 
 int dont_trigger() {
   unsigned p;
   p = Q | P;
int dont_trigger() {
   if (Tuesday != Monday + 1 ||
       Friday - Thursday != 1 ||
       Sunday + Wednesday == (Sunday | Wednesday))
     return 1;
   if (H + I + L == 42)
     return 1;
   return 42;
 }
+
+namespace PR34400 {
+enum { E1 = 0 };
+enum { E2 = -1 };
+enum { l = E1 | E2 };
+}