[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:58
+  // Interfaces should have exclusively pure methods.
+  for (auto method : Node->methods()) {
+    if (method->isUserProvided() && !method->isPure()) {
----------------
Eugene.Zelenko wrote:
> const auto?
Should be `const auto *`; also `method` does not meet our naming convention rules. Actually, the whole thing can be replaced by `return llvm::none_of(Node->methods(), [](const CXXMethodDecl *M) { return M->isUserProvided() && !M->isPure(); });`


================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:21
+
+AST_MATCHER(CXXRecordDecl, hasDefinition) {
+  if (!Node.hasDefinition())
----------------
This is another example of an AST matcher that should simply be made public in ASTMatchers.h (as a separate commit). Feel free to add me as a reviewer to that patch (it should be pretty trivial).


================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:29-30
+// previously.
+void MultipleInheritanceCheck::addNodeToInterfaceMap(const CXXRecordDecl *Node,
+                            bool isInterface) {
+  StringRef Name = Node->getIdentifier()->getName();
----------------
Formatting is off here -- be sure to run the patch through clang-format.


================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:40
+bool MultipleInheritanceCheck::getInterfaceStatus(const CXXRecordDecl *Node,
+                            bool *isInterface) {
+  StringRef Name = Node->getIdentifier()->getName();
----------------
Rather than passing a pointer, why not pass a reference?


================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:49
+
+// TODO(smklein): Make an exception for 'Dispatcher' -- consider it an
+// interface, even though it isn't.
----------------
We don't put names into our TODO or FIXME comments.


================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:54
+  // Interfaces should have no fields.
+  if (!Node->field_empty()) {
+    return false;
----------------
Can elide braces, here and elsewhere.


================
Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:102
+    // concrete classes
+    int NumConcrete = 0;
+    for (const auto &I : D->bases()) {
----------------
Might as well make this `unsigned` rather than `int`.


https://reviews.llvm.org/D40580