[PATCH] D43392: [clang-tidy] Add Fuchsia checker for visibility attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:32
+  if (Name.empty()) return;
+  Finder->addMatcher(functionDecl(allOf(hasName(Name), isDefinition(),
+                                        unless(hasVisibilityAttr())))
----------------
Would it make more sense to store a list of names and then find all of them at once, rather than a single name at a time?


================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:34
+                                        unless(hasVisibilityAttr())))
+                         .bind("x"),
+                     this);
----------------
It'd be nice to pick a slightly better string literal to use.


================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:40
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<NamedDecl>("x");
+  diag(MatchedDecl->getLocStart(), "set visibility attr to hidden")
+      << MatchedDecl
----------------
This diagnostic doesn't really tell the user what's wrong with their code.


================
Comment at: test/clang-tidy/fuchsia-add-visibility.cpp:10
+// CHECK-FIXES: __attribute__((visibility("hidden")))
+
----------------
I'd also like to see tests where the attribute is already present on the definition (to ensure it doesn't get added), or is already present on the declaration and is inherited on the definition.

What should happen with code like this:
```
__attribute__((visibility("internal"))) void f();

void f() {

}
```
Will this check attempt to add `__attribute__((visibility("hidden")))` to the definition?


https://reviews.llvm.org/D43392