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

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.


================
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>();
----------------
aaron.ballman wrote:
> What about virtual bases (`Node->vbases()`)? This would also be worth some test cases.
Added test cases for virtual, but aren't virtual bases also included in `bases()`?


================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:77
+  // Match declarations which have definitions.
+  Finder->addMatcher(cxxRecordDecl(hasDefinition()).bind("decl"), this);
+}
----------------
aaron.ballman wrote:
> 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).
Good point -- will do.


================
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);
----------------
aaron.ballman wrote:
> I believe all these methods can be marked `const`.
getInterfaceStatus and isInterface can't be -- they update the map. The other ones yes though!


https://reviews.llvm.org/D40580