alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.
================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:22
+AST_MATCHER(CXXRecordDecl, hasDefinition) {
+ if (!Node.hasDefinition())
+ return false;
----------------
`return Node.hasDefinition();`
================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:32
+ StringRef Name = Node->getIdentifier()->getName();
+ MultipleInheritanceCheck::InterfaceMap->insert(
+ std::make_pair(Name, isInterface));
----------------
What's the reason to qualify `InterfaceMap` and other members of this class?
================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.h:35-38
+ void addNodeToInterfaceMap(const CXXRecordDecl *Node, bool isInterface);
+ bool getInterfaceStatus(const CXXRecordDecl *Node, bool *isInterface);
+ bool isCurrentClassInterface(const CXXRecordDecl *Node);
+ bool isInterface(const CXXRecordDecl *Node);
----------------
Do all these methods need to be public?
================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.h:44
+ // where N is the number of classes.
+ llvm::StringMap<bool> *InterfaceMap;
+};
----------------
What's the reason to use a pointer and dynamically allocate the map? Just make
it a value and clear it instead of deleting.
================
Comment at: docs/clang-tidy/checks/fuchsia-multiple-inheritance.rst:46
+
+See the features disallowed in Fuchsia at
https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md
----------------
This is not about the check, rather about the underlying style guide. The
document linked here doesn't explain why certain features are disallowed. I'd
suggest putting some effort in expanding the document to include reasoning for
each rule (e.g. see
https://google.github.io/styleguide/cppguide.html#Multiple_Inheritance for a
related rule in the Google C++ style guide).
https://reviews.llvm.org/D40580
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
