[PATCH] D37572: [clang-tidy] SuspiciousEnumUsageCheck bugfix

[Peter Szecsi via Phabricator via cfe-commits]

szepet created this revision.
Herald added subscribers: baloghadamsoftware, whisperity, JDevlieghere.

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?


https://reviews.llvm.org/D37572

Files:
  clang-tidy/misc/SuspiciousEnumUsageCheck.cpp
  test/clang-tidy/misc-suspicious-enum-usage.cpp


Index: test/clang-tidy/misc-suspicious-enum-usage.cpp
===================================================================
--- test/clang-tidy/misc-suspicious-enum-usage.cpp
+++ test/clang-tidy/misc-suspicious-enum-usage.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 0}]}" --
+// RUN: %check_clang_tidy %s misc-suspicious-enum-usage %t -- -config="{CheckOptions: [{key: misc-suspicious-enum-usage.StrictMode, value: 0}]}" -- -std=c++11
 
 enum Empty {
 };
@@ -54,7 +54,7 @@
   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
@@ -88,3 +88,41 @@
     return 1;
   return 42;
 }
+
+// Bug #34400
+template <typename>
+struct a;
+template <typename, int, int b, int = 0, int = 0, int = b>
+struct c;
+template <typename>
+struct e;
+template <typename, typename, int>
+struct f;
+template <typename g>
+struct h {
+  typedef e<g> i;
+};
+template <typename g>
+struct j {
+  enum { k = a<g>::l,
+         m };
+};
+template <typename g>
+struct e : j<g> {};
+template <typename g>
+struct r : h<g>::i {};
+template <typename n, int d, int b, int o, int p, int q>
+struct a<c<n, d, b, o, p, q>> {
+  enum { l = q };
+};
+template <typename, int, int, int, int, int q>
+struct c : r<c<int, 0, q>> {};
+template <typename ad, typename ae, int af>
+struct a<f<ad, ae, af>> {
+  enum { ah = ad::m,
+         ai = ae::m,
+         l = ah | ai };
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: enum values are from different enum types
+};
+template <typename, typename, int>
+struct f : e<f<c<int, 0, 0>, c<int, 0, -1>, 0>> {};
Index: clang-tidy/misc/SuspiciousEnumUsageCheck.cpp
===================================================================
--- clang-tidy/misc/SuspiciousEnumUsageCheck.cpp
+++ clang-tidy/misc/SuspiciousEnumUsageCheck.cpp
@@ -42,7 +42,8 @@
     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()) == -1;
         });
     MinVal = MinMaxVal.first->getInitVal();
     MaxVal = MinMaxVal.second->getInitVal();
@@ -57,7 +58,9 @@
 static bool hasDisjointValueRange(const EnumDecl *Enum1,
                                   const EnumDecl *Enum2) {
   ValueRange Range1(Enum1), Range2(Enum2);
-  return (Range1.MaxVal < Range2.MinVal) || (Range2.MaxVal < Range1.MinVal);
+  bool Less1 = llvm::APSInt::compareValues(Range1.MaxVal, Range2.MinVal) == -1;
+  bool Less2 = llvm::APSInt::compareValues(Range2.MaxVal, Range1.MinVal) == -1;
+  return Less1 || Less2;
 }
 
 static bool isNonPowerOf2NorNullLiteral(const EnumConstantDecl *EnumConst) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D37572.114180.patch
Type: text/x-patch
Size: 3193 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170907/e2e1655f/attachment-0001.bin>