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

[Alexander Kornienko via Phabricator via cfe-commits]

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