Re-enable the clang check for enum constants.

The patch adds a flag to disable the feature, so as to avoid future reverts.
There is also a check to avoid mixed signed comparisons, as can
arise with enums from C files.  We need not bother checking these.

BUG=356816
R=mark@chromium.org
R=thakis@chromium.org

Re-enable flag.


patch from issue 150943005

Review URL: https://codereview.chromium.org/213403010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@260249 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 172 insertions, 1 deletions

diff --git a/tools/clang/plugins/ChromeClassTester.cpp b/tools/clang/plugins/ChromeClassTester.cpp
index e788c15..5ce04e5 100644
--- a/tools/clang/plugins/ChromeClassTester.cpp
+++ b/tools/clang/plugins/ChromeClassTester.cpp
@@ -91,6 +91,16 @@ void ChromeClassTester::CheckTag(TagDecl* tag) {
         return;
 
     CheckChromeClass(record_location, record);
+  } else if (EnumDecl* enum_decl = dyn_cast<EnumDecl>(tag)) {
+    SourceLocation enum_location = enum_decl->getInnerLocStart();
+    if (InBannedDirectory(enum_location))
+      return;
+
+    std::string base_name = enum_decl->getNameAsString();
+    if (IsIgnoredType(base_name))
+      return;
+
+    CheckChromeEnum(enum_location, enum_decl);
   }
 }
 
@@ -195,6 +205,9 @@ void ChromeClassTester::BuildBannedLists() {
   // non-pod class member. Probably harmless.
   ignored_record_names_.insert("MockTransaction");
 
+  // Enum type with _LAST members where _LAST doesn't mean last enum value.
+  ignored_record_names_.insert("ServerFieldType");
+
   // Used heavily in ui_unittests and once in views_unittests. Fixing this
   // isn't worth the overhead of an additional library.
   ignored_record_names_.insert("TestAnimationDelegate");
@@ -206,6 +219,9 @@ void ChromeClassTester::BuildBannedLists() {
   // Measured performance improvement on cc_perftests. See
   // https://codereview.chromium.org/11299290/
   ignored_record_names_.insert("QuadF");
+
+  // Enum type with _LAST members where _LAST doesn't mean last enum value.
+  ignored_record_names_.insert("ViewID");
 }
 
 std::string ChromeClassTester::GetNamespaceImpl(const DeclContext* context,
diff --git a/tools/clang/plugins/ChromeClassTester.h b/tools/clang/plugins/ChromeClassTester.h
index 588ae9c..6bd19a6 100644
--- a/tools/clang/plugins/ChromeClassTester.h
+++ b/tools/clang/plugins/ChromeClassTester.h
@@ -54,6 +54,12 @@ class ChromeClassTester : public clang::ASTConsumer {
   virtual void CheckChromeClass(clang::SourceLocation record_location,
                                 clang::CXXRecordDecl* record) = 0;
 
+  // Filtered versions of enum type that are only called with things defined
+  // in chrome header files.
+  virtual void CheckChromeEnum(clang::SourceLocation enum_location,
+                               clang::EnumDecl* enum_decl) {
+  }
+
   // Utility methods used for filtering out non-chrome classes (and ones we
   // deliberately ignore) in HandleTagDeclDefinition().
   std::string GetNamespaceImpl(const clang::DeclContext* context,
diff --git a/tools/clang/plugins/FindBadConstructs.cpp b/tools/clang/plugins/FindBadConstructs.cpp
index aa8e813..4b74fbe 100644
--- a/tools/clang/plugins/FindBadConstructs.cpp
+++ b/tools/clang/plugins/FindBadConstructs.cpp
@@ -14,6 +14,8 @@
 //   should have protected or private destructors.
 // - WeakPtrFactory members that refer to their outer class should be the last
 //   member.
+// - Enum types with a xxxx_LAST or xxxxLast const actually have that constant
+//   have the maximal value for that type.
 
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/AST.h"
@@ -48,6 +50,9 @@ const char kProtectedNonVirtualDtor[] =
 const char kWeakPtrFactoryOrder[] =
     "[chromium-style] WeakPtrFactory members which refer to their outer class "
     "must be the last member in the outer class definition.";
+const char kBadLastEnumValue[] =
+    "[chromium-style] _LAST/Last constants of enum types must have the maximal "
+    "value for any constant of that type.";
 const char kNoteInheritance[] =
     "[chromium-style] %0 inherits from %1 here";
 const char kNoteImplicitDtor[] =
@@ -78,11 +83,13 @@ const Type* UnwrapType(const Type* type) {
 struct FindBadConstructsOptions {
   FindBadConstructsOptions() : check_base_classes(false),
                                check_virtuals_in_implementations(true),
-                               check_weak_ptr_factory_order(false) {
+                               check_weak_ptr_factory_order(false),
+                               check_enum_last_value(false) {
   }
   bool check_base_classes;
   bool check_virtuals_in_implementations;
   bool check_weak_ptr_factory_order;
+  bool check_enum_last_value;
 };
 
 // Searches for constructs that we know we don't want in the Chromium code base.
@@ -105,6 +112,8 @@ class FindBadConstructsConsumer : public ChromeClassTester {
         getErrorLevel(), kProtectedNonVirtualDtor);
     diag_weak_ptr_factory_order_ = diagnostic().getCustomDiagID(
         getErrorLevel(), kWeakPtrFactoryOrder);
+    diag_bad_enum_last_value_ = diagnostic().getCustomDiagID(
+        getErrorLevel(), kBadLastEnumValue);
 
     // Registers notes to make it easier to interpret warnings.
     diag_note_inheritance_ = diagnostic().getCustomDiagID(
@@ -141,6 +150,45 @@ class FindBadConstructsConsumer : public ChromeClassTester {
       CheckWeakPtrFactoryMembers(record_location, record);
   }
 
+  virtual void CheckChromeEnum(SourceLocation enum_location,
+                               EnumDecl* enum_decl) {
+    if (!options_.check_enum_last_value)
+      return;
+
+    bool got_one = false;
+    bool is_signed = false;
+    llvm::APSInt max_so_far;
+    EnumDecl::enumerator_iterator iter;
+    for (iter = enum_decl->enumerator_begin();
+         iter != enum_decl->enumerator_end(); ++iter) {
+          llvm::APSInt current_value = iter->getInitVal();
+      if (!got_one) {
+        max_so_far = current_value;
+        is_signed = current_value.isSigned();
+        got_one = true;
+      } else {
+        if (is_signed != current_value.isSigned()) {
+          // This only happens in some cases when compiling C (not C++) files,
+          // so it is OK to bail out here.
+          return;
+        }
+        if (current_value > max_so_far)
+          max_so_far = current_value;
+      }
+    }
+    for (iter = enum_decl->enumerator_begin();
+         iter != enum_decl->enumerator_end(); ++iter) {
+      std::string name = iter->getNameAsString();
+      if (((name.size() > 4 &&
+            name.compare(name.size() - 4, 4, "Last") == 0) ||
+           (name.size() > 5 &&
+            name.compare(name.size() - 5, 5, "_LAST") == 0)) &&
+          iter->getInitVal() < max_so_far) {
+        diagnostic().Report(iter->getLocation(), diag_bad_enum_last_value_);
+      }
+    }
+  }
+
  private:
   // The type of problematic ref-counting pattern that was encountered.
   enum RefcountIssue {
@@ -157,6 +205,7 @@ class FindBadConstructsConsumer : public ChromeClassTester {
   unsigned diag_public_dtor_;
   unsigned diag_protected_non_virtual_dtor_;
   unsigned diag_weak_ptr_factory_order_;
+  unsigned diag_bad_enum_last_value_;
   unsigned diag_note_inheritance_;
   unsigned diag_note_implicit_dtor_;
   unsigned diag_note_public_dtor_;
@@ -707,6 +756,10 @@ class FindBadConstructsAction : public PluginASTAction {
       } else if (args[i] == "check-weak-ptr-factory-order") {
         // TODO(dmichael): Remove this once http://crbug.com/303818 is fixed.
         options_.check_weak_ptr_factory_order = true;
+      } else if (args[i] == "check-enum-last-value") {
+        // TODO(tsepez): Enable this by default once http://crbug.com/356815
+        // and http://crbug.com/356816 are fixed.
+        options_.check_enum_last_value = true;
       } else {
         parsed = false;
         llvm::errs() << "Unknown clang plugin argument: " << args[i] << "\n";
diff --git a/tools/clang/plugins/tests/enum_last_value.cpp b/tools/clang/plugins/tests/enum_last_value.cpp
new file mode 100644
index 0000000..c189f23
--- /dev/null
+++ b/tools/clang/plugins/tests/enum_last_value.cpp
@@ -0,0 +1,39 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// We warn when xxxLAST constants aren't last.
+enum BadOne {
+  kBadOneInvalid = -1,
+  kBadOneRed,
+  kBadOneGreen,
+  kBadOneBlue,
+  kBadOneLast = kBadOneGreen
+};
+
+// We warn when xxx_LAST constants aren't last.
+enum BadTwo {
+  BAD_TWO_INVALID,
+  BAD_TWO_RED,
+  BAD_TWO_GREEN,
+  BAD_TWO_BLUE = 0xfffffffc,
+  BAD_TWO_LAST = BAD_TWO_GREEN
+};
+
+// We don't warn when xxxLAST constants are last.
+enum GoodOne {
+  kGoodOneInvalid = -1,
+  kGoodOneRed,
+  kGoodOneGreen,
+  kGoodOneBlue,
+  kGoodOneLast = kGoodOneBlue
+};
+
+// We don't warn when xxx_LAST constants are last.
+enum GoodTwo {
+  GOOD_TWO_INVALID,
+  GOOD_TWO_RED,
+  GOOD_TWO_GREEN,
+  GOOD_TWO_BLUE = 0xfffffffc,
+  GOOD_TWO_LAST = GOOD_TWO_BLUE
+};
diff --git a/tools/clang/plugins/tests/enum_last_value.flags b/tools/clang/plugins/tests/enum_last_value.flags
new file mode 100644
index 0000000..ee2ef7e
--- /dev/null
+++ b/tools/clang/plugins/tests/enum_last_value.flags
@@ -0,0 +1 @@
+-Xclang -plugin-arg-find-bad-constructs -Xclang check-enum-last-value
diff --git a/tools/clang/plugins/tests/enum_last_value.txt b/tools/clang/plugins/tests/enum_last_value.txt
new file mode 100644
index 0000000..2d9e51d
--- /dev/null
+++ b/tools/clang/plugins/tests/enum_last_value.txt
@@ -0,0 +1,7 @@
+enum_last_value.cpp:11:3: warning: [chromium-style] _LAST/Last constants of enum types must have the maximal value for any constant of that type.
+  kBadOneLast = kBadOneGreen
+  ^
+enum_last_value.cpp:20:3: warning: [chromium-style] _LAST/Last constants of enum types must have the maximal value for any constant of that type.
+  BAD_TWO_LAST = BAD_TWO_GREEN
+  ^
+2 warnings generated.
diff --git a/tools/clang/plugins/tests/enum_last_value_from_c.c b/tools/clang/plugins/tests/enum_last_value_from_c.c
new file mode 100644
index 0000000..7fecbc0
--- /dev/null
+++ b/tools/clang/plugins/tests/enum_last_value_from_c.c
@@ -0,0 +1,40 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// We warn when xxxLAST constants aren't last.
+enum BadOne {
+  kBadOneInvalid = -1,
+  kBadOneRed,
+  kBadOneGreen,
+  kBadOneBlue,
+  kBadOneLast = kBadOneGreen
+};
+
+// We don't handle this case when called from C due to sign mismatch issues.
+// No matter; we're not looking for this issue outside of C++.
+enum FailOne {
+  FAIL_ONE_INVALID,
+  FAIL_ONE_RED,
+  FAIL_ONE_GREEN,
+  FAIL_ONE_BLUE = 0xfffffffc,
+  FAIL_ONE_LAST = FAIL_ONE_GREEN
+};
+
+// We don't warn when xxxLAST constants are last.
+enum GoodOne {
+  kGoodOneInvalid = -1,
+  kGoodOneRed,
+  kGoodOneGreen,
+  kGoodOneBlue,
+  kGoodOneLast = kGoodOneBlue
+};
+
+// We don't warn when xxx_LAST constants are last.
+enum GoodTwo {
+  GOOD_TWO_INVALID,
+  GOOD_TWO_RED,
+  GOOD_TWO_GREEN,
+  GOOD_TWO_BLUE = 0xfffffffc,
+  GOOD_TWO_LAST = GOOD_TWO_BLUE
+};
diff --git a/tools/clang/plugins/tests/enum_last_value_from_c.flags b/tools/clang/plugins/tests/enum_last_value_from_c.flags
new file mode 100644
index 0000000..ee2ef7e
--- /dev/null
+++ b/tools/clang/plugins/tests/enum_last_value_from_c.flags
@@ -0,0 +1 @@
+-Xclang -plugin-arg-find-bad-constructs -Xclang check-enum-last-value
diff --git a/tools/clang/plugins/tests/enum_last_value_from_c.txt b/tools/clang/plugins/tests/enum_last_value_from_c.txt
new file mode 100644
index 0000000..3aa1d16
--- /dev/null
+++ b/tools/clang/plugins/tests/enum_last_value_from_c.txt
@@ -0,0 +1,4 @@
+enum_last_value_from_c.c:11:3: warning: [chromium-style] _LAST/Last constants of enum types must have the maximal value for any constant of that type.
+  kBadOneLast = kBadOneGreen
+  ^
+1 warning generated.
diff --git a/tools/clang/plugins/tests/test.sh b/tools/clang/plugins/tests/test.sh
index cf3acc3..1a27f48 100755
--- a/tools/clang/plugins/tests/test.sh
+++ b/tools/clang/plugins/tests/test.sh
@@ -71,6 +71,10 @@ for input in *.cpp; do
   do_testcase "${input}" "${input%cpp}txt" "${input%cpp}flags"
 done
 
+for input in *.c; do
+  do_testcase "${input}" "${input%c}txt" "${input%c}flags"
+done
+
 if [[ "${failed_any_test}" ]]; then
   exit ${E_FAILEDTEST}
 fi