Bug 1244825 - Detect classes with two superclasses with mRefCnt members, r=ehsan
authorMichael Layzell <michael@thelayzells.com>
Thu, 04 Aug 2016 12:54:25 -0400
changeset 397517 4ed23ebeb25b01a2b43cfb78683a88700eced6a4
parent 397516 fb4fa13ea19aa28010329f15aed7bd1dd0f5ee54
child 397518 0f7ec83d606e646f6c8732f21702361023bdd5dc
push id25332
push usermaglione.k@gmail.com
push dateSat, 06 Aug 2016 21:21:51 +0000
reviewersehsan
bugs1244825
milestone51.0a1
Bug 1244825 - Detect classes with two superclasses with mRefCnt members, r=ehsan
build/clang-plugin/clang-plugin.cpp
build/clang-plugin/tests/TestNoDuplicateRefCntMember.cpp
--- a/build/clang-plugin/clang-plugin.cpp
+++ b/build/clang-plugin/clang-plugin.cpp
@@ -1075,19 +1075,17 @@ DiagnosticsMatcher::DiagnosticsMatcher()
   // Older clang versions such as the ones used on the infra recognize these
   // conversions as 'operator _Bool', but newer clang versions recognize these
   // as 'operator bool'.
   astMatcher.addMatcher(
       cxxMethodDecl(anyOf(hasName("operator bool"), hasName("operator _Bool")))
           .bind("node"),
       &explicitOperatorBoolChecker);
 
-  astMatcher.addMatcher(
-      cxxRecordDecl(allOf(decl().bind("decl"), hasRefCntMember())),
-      &noDuplicateRefCntMemberChecker);
+  astMatcher.addMatcher(cxxRecordDecl().bind("decl"), &noDuplicateRefCntMemberChecker);
 
   astMatcher.addMatcher(
       classTemplateSpecializationDecl(
           allOf(hasAnyTemplateArgument(refersToType(hasVTable())),
                 hasNeedsNoVTableTypeAttr()))
           .bind("node"),
       &needsNoVTableTypeChecker);
 
@@ -1433,41 +1431,72 @@ void DiagnosticsMatcher::ExplicitOperato
     Diag.Report(method->getLocStart(), errorID) << clazz;
     Diag.Report(method->getLocStart(), noteID) << "'operator bool'";
   }
 }
 
 void DiagnosticsMatcher::NoDuplicateRefCntMemberChecker::run(
     const MatchFinder::MatchResult &Result) {
   DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
-  unsigned warningID = Diag.getDiagnosticIDs()->getCustomDiagID(
-      DiagnosticIDs::Error,
-      "Refcounted record %0 has multiple mRefCnt members");
-  unsigned note1ID = Diag.getDiagnosticIDs()->getCustomDiagID(
-      DiagnosticIDs::Note, "Superclass %0 also has an mRefCnt member");
-  unsigned note2ID = Diag.getDiagnosticIDs()->getCustomDiagID(
-      DiagnosticIDs::Note,
-      "Consider using the _INHERITED macros for AddRef and Release here");
+  const CXXRecordDecl *D = Result.Nodes.getNodeAs<CXXRecordDecl>("decl");
+  const FieldDecl *refCntMember = getClassRefCntMember(D);
+  const FieldDecl *foundRefCntBase = nullptr;
 
-  const CXXRecordDecl *decl = Result.Nodes.getNodeAs<CXXRecordDecl>("decl");
-  const FieldDecl *refCntMember = getClassRefCntMember(decl);
-  assert(refCntMember &&
-         "The matcher checked to make sure we have a refCntMember");
+  if (!D->hasDefinition())
+    return;
+  D = D->getDefinition();
+
+  // If we don't have an mRefCnt member, and we have less than 2 superclasses,
+  // we don't have to run this loop, as neither case will ever apply.
+  if (!refCntMember && D->getNumBases() < 2) {
+    return;
+  }
 
   // Check every superclass for whether it has a base with a refcnt member, and
   // warn for those which do
-  for (CXXRecordDecl::base_class_const_iterator base = decl->bases_begin(),
-                                                e = decl->bases_end();
-       base != e; ++base) {
-    const FieldDecl *baseRefCntMember = getBaseRefCntMember(base->getType());
+  for (auto &base : D->bases()) {
+    // Determine if this base class has an mRefCnt member
+    const FieldDecl *baseRefCntMember = getBaseRefCntMember(base.getType());
+
     if (baseRefCntMember) {
-      Diag.Report(decl->getLocStart(), warningID) << decl;
-      Diag.Report(baseRefCntMember->getLocStart(), note1ID)
+      if (refCntMember) {
+        // We have an mRefCnt, and superclass has an mRefCnt
+        unsigned error = Diag.getDiagnosticIDs()->getCustomDiagID(
+            DiagnosticIDs::Error,
+            "Refcounted record %0 has multiple mRefCnt members");
+        unsigned note1 = Diag.getDiagnosticIDs()->getCustomDiagID(
+            DiagnosticIDs::Note, "Superclass %0 also has an mRefCnt member");
+        unsigned note2 = Diag.getDiagnosticIDs()->getCustomDiagID(
+            DiagnosticIDs::Note,
+            "Consider using the _INHERITED macros for AddRef and Release here");
+
+        Diag.Report(D->getLocStart(), error) << D;
+        Diag.Report(baseRefCntMember->getLocStart(), note1)
           << baseRefCntMember->getParent();
-      Diag.Report(refCntMember->getLocStart(), note2ID);
+        Diag.Report(refCntMember->getLocStart(), note2);
+      }
+
+      if (foundRefCntBase) {
+        unsigned error = Diag.getDiagnosticIDs()->getCustomDiagID(
+            DiagnosticIDs::Error,
+            "Refcounted record %0 has multiple superclasses with mRefCnt members");
+        unsigned note = Diag.getDiagnosticIDs()->getCustomDiagID(
+            DiagnosticIDs::Note,
+            "Superclass %0 has an mRefCnt member");
+
+        // superclass has mRefCnt, and another superclass also has an mRefCnt
+        Diag.Report(D->getLocStart(), error) << D;
+        Diag.Report(baseRefCntMember->getLocStart(), note)
+          << baseRefCntMember->getParent();
+        Diag.Report(foundRefCntBase->getLocStart(), note)
+          << foundRefCntBase->getParent();
+      }
+
+      // Record that we've found a base with a mRefCnt member
+      foundRefCntBase = baseRefCntMember;
     }
   }
 }
 
 void DiagnosticsMatcher::NeedsNoVTableTypeChecker::run(
     const MatchFinder::MatchResult &Result) {
   DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
   unsigned errorID = Diag.getDiagnosticIDs()->getCustomDiagID(
--- a/build/clang-plugin/tests/TestNoDuplicateRefCntMember.cpp
+++ b/build/clang-plugin/tests/TestNoDuplicateRefCntMember.cpp
@@ -1,38 +1,49 @@
 class C1 {};
 
 class RC1 {
 public:
   virtual void AddRef();
   virtual void Release();
 
 private:
-  int mRefCnt; // expected-note 2 {{Superclass 'RC1' also has an mRefCnt member}}
+  int mRefCnt; // expected-note 2 {{Superclass 'RC1' also has an mRefCnt member}} expected-note 3 {{Superclass 'RC1' has an mRefCnt member}}
 };
 
 class RC2 : public RC1 { // expected-error {{Refcounted record 'RC2' has multiple mRefCnt members}}
 public:
   virtual void AddRef();
   virtual void Release();
 
 private:
   int mRefCnt; // expected-note {{Consider using the _INHERITED macros for AddRef and Release here}}
 };
 
 class C2 : public RC1 {};
 
 class RC3 : public RC1 {};
 
-class RC4 : public RC3, public C2 {};
+class RC4 : public RC3, public C2 {}; // expected-error {{Refcounted record 'RC4' has multiple superclasses with mRefCnt members}}
 
 class RC5 : public RC1 {};
 
 class RC6 : public C1, public RC5 { // expected-error {{Refcounted record 'RC6' has multiple mRefCnt members}}
 public:
   virtual void AddRef();
   virtual void Release();
 
 private:
   int mRefCnt; // expected-note {{Consider using the _INHERITED macros for AddRef and Release here}}
 };
 
 class Predecl;
+
+class OtherRC {
+public:
+  virtual void AddRef();
+  virtual void Release();
+
+private:
+  int mRefCnt; // expected-note {{Superclass 'OtherRC' has an mRefCnt member}}
+};
+
+class MultRCSuper : public RC1, public OtherRC {}; // expected-error {{Refcounted record 'MultRCSuper' has multiple superclasses with mRefCnt members}}