[PATCH] D40580: [clang-tidy] Adding Fuchsia checker for multiple inheritance

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:35
+  StringRef Name = Node->getIdentifier()->getName();
+  auto Pair = InterfaceMap.find(Name);
+  if (Pair == InterfaceMap.end())
----------------
Don't use `auto` as the type is not spelled out explicitly in the initialization.


================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:60
+  // To be an interface, all base classes must be interfaces as well.
+  for (const auto &I : Node->bases()) {
+    const auto *Ty = I.getType()->getAs<RecordType>();
----------------
What about virtual bases (`Node->vbases()`)? This would also be worth some test cases.


================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:77
+  // Match declarations which have definitions.
+  Finder->addMatcher(cxxRecordDecl(hasDefinition()).bind("decl"), this);
+}
----------------
It might be nice to not bother matching class definitions that have no bases or virtual bases rather than matching every class definition. However, this could be done in a follow-up patch (it likely requires adding an AST matcher).


================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:85
+    unsigned NumConcrete = 0;
+    for (const auto &I : D->bases()) {
+      const auto *Ty = I.getType()->getAs<RecordType>();
----------------
And virtual bases?


================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:88
+      assert(Ty && "RecordType of base class is unknown");
+      const auto *Base = cast<CXXRecordDecl>(Ty->getDecl()->getDefinition());
+      if (!isInterface(Base)) NumConcrete++;
----------------
It might make sense to add a degenerate case here to ensure a base class without a definition doesn't cause a null pointer dereference. e.g.,
```
struct B;

struct D : B {}; // compile error, B is not a complete type
```
I'm not certain whether Clang's AST will contain the base or not.


================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.h:34-36
+  bool getInterfaceStatus(const CXXRecordDecl *Node, bool &isInterface);
+  bool isCurrentClassInterface(const CXXRecordDecl *Node);
+  bool isInterface(const CXXRecordDecl *Node);
----------------
I believe all these methods can be marked `const`.


================
Comment at: test/clang-tidy/fuchsia-multiple-inheritance.cpp:80
+};
+
+int main(void) {
----------------
Please add a test case consisting of only data members, e.g.,
```
struct B1 {
  int x;
};

struct B2 {
  int x;
};

struct D : B1, B2 {};
```


https://reviews.llvm.org/D40580