Re-enable the clang check for enum constants.

tools/clang/plugins/FindBadConstructs.cpp

 // Copyright (c) 2012 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.
 
 // This file defines a bunch of recurring problems in the Chromium C++ code.
 //
 // Checks that are implemented:
 // - Constructors/Destructors should not be inlined if they are of a complex
 //   class type.
 // - Missing "virtual" keywords on methods that should be virtual.
 // - Non-annotated overriding virtual methods.
 // - Virtual methods with nonempty implementations in their headers.
 // - Classes that derive from base::RefCounted / base::RefCountedThreadSafe
 //   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"
 #include "clang/AST/Attr.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendPluginRegistry.h"
 #include "clang/Lex/Lexer.h"
@@ skipping 14 matching lines @@
     "destructors that are declared protected or private.";
 const char kPublicDtor[] =
     "[chromium-style] Classes that are ref-counted should have "
     "destructors that are declared protected or private.";
 const char kProtectedNonVirtualDtor[] =
     "[chromium-style] Classes that are ref-counted and have non-private "
     "destructors should declare their destructor virtual.";
 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[] =
     "[chromium-style] No explicit destructor for %0 defined";
 const char kNotePublicDtor[] =
     "[chromium-style] Public destructor declared here";
 const char kNoteProtectedNonVirtualDtor[] =
     "[chromium-style] Protected non-virtual destructor declared here";
 
 bool TypeHasNonTrivialDtor(const Type* type) {
@@ skipping 10 matching lines @@
   if (const ElaboratedType* elaborated = dyn_cast<ElaboratedType>(type))
     return UnwrapType(elaborated->getNamedType().getTypePtr());
   if (const TypedefType* typedefed = dyn_cast<TypedefType>(type))
     return UnwrapType(typedefed->desugar().getTypePtr());
   return 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(true) {
   }
   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.
 class FindBadConstructsConsumer : public ChromeClassTester {
  public:
   FindBadConstructsConsumer(CompilerInstance& instance,
                             const FindBadConstructsOptions& options)
       : ChromeClassTester(instance),
         options_(options) {
     // Register warning/error messages.
     diag_method_requires_override_ = diagnostic().getCustomDiagID(
         getErrorLevel(), kMethodRequiresOverride);
     diag_method_requires_virtual_ = diagnostic().getCustomDiagID(
         getErrorLevel(), kMethodRequiresVirtual);
     diag_no_explicit_dtor_ = diagnostic().getCustomDiagID(
         getErrorLevel(), kNoExplicitDtor);
     diag_public_dtor_ = diagnostic().getCustomDiagID(
         getErrorLevel(), kPublicDtor);
     diag_protected_non_virtual_dtor_ = diagnostic().getCustomDiagID(
         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(
         DiagnosticsEngine::Note, kNoteInheritance);
     diag_note_implicit_dtor_ = diagnostic().getCustomDiagID(
         DiagnosticsEngine::Note, kNoteImplicitDtor);
     diag_note_public_dtor_ = diagnostic().getCustomDiagID(
         DiagnosticsEngine::Note, kNotePublicDtor);
     diag_note_protected_non_virtual_dtor_ = diagnostic().getCustomDiagID(
         DiagnosticsEngine::Note, kNoteProtectedNonVirtualDtor);
@@ skipping 16 matching lines @@
       // virtual and OVERRIDE.
       CheckVirtualMethods(record_location, record, warn_on_inline_bodies);
     }
 
     CheckRefCountedDtors(record_location, record);
 
     if (options_.check_weak_ptr_factory_order)
       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 {
     None,
     ImplicitDestructor,
     PublicDestructor
   };
 
   FindBadConstructsOptions options_;
 
   unsigned diag_method_requires_override_;
   unsigned diag_method_requires_virtual_;
   unsigned diag_no_explicit_dtor_;
   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_;
   unsigned diag_note_protected_non_virtual_dtor_;
 
   // Prints errors if the constructor/destructor weight is too heavy.
   void CheckCtorDtorWeight(SourceLocation record_location,
                            CXXRecordDecl* record) {
     // We don't handle anonymous structs. If this record doesn't have a
     // name, it's of the form:
@@ skipping 530 matching lines @@
     for (size_t i = 0; i < args.size() && parsed; ++i) {
       if (args[i] == "skip-virtuals-in-implementations") {
         // TODO(rsleevi): Remove this once http://crbug.com/115047 is fixed.
         options_.check_virtuals_in_implementations = false;
       } else if (args[i] == "check-base-classes") {
         // TODO(rsleevi): Remove this once http://crbug.com/123295 is fixed.
         options_.check_base_classes = true;
       } 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] == "skip-enum-last-value") {

[Nico, 2014/03/27 22:53:46]

Usually, new warnings default to off, and are turned on after a clang roll has
safely landed and been in the tree for a while, to make the roll easier.

+        // TODO(tsepez): Remove this once http://crbug.com/356815 and
+        // http://crbug.com/356816 are fixed.
+        options_.check_enum_last_value = false;
       } else {
         parsed = false;
         llvm::errs() << "Unknown clang plugin argument: " << args[i] << "\n";
       }
     }
 
     return parsed;
   }
 
  private:
   FindBadConstructsOptions options_;
 };
 
 }  // namespace
 
 static FrontendPluginRegistry::Add<FindBadConstructsAction>
 X("find-bad-constructs", "Finds bad C++ constructs");