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