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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits