aaron.ballman added inline comments.
================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:36-39
+ StringRef(Options.get("Names", "")).split(NamesRefs, ",");
+ for (const auto &Name : NamesRefs) {
+ if (!Name.empty()) Names.push_back(Name);
+ }
----------------
Rather than do this, you should use `parseStringList()` from `OptionsUtils.h`.
This will also ensure you don't use the wrong separator (we use semi-colons for
this, not commas).
================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:48
+ Stream << Names[0];
+ for (unsigned i = 1; i < Names.size(); ++i)
+ Stream << "," << Names[i];
----------------
You should use `serializeStringList()` for this.
See `InefficientVectorOperationCheck` for an example of parsing and serializing
the string lists.
================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:56
+ for (const auto &Name : Names)
+ Finder->addMatcher(functionDecl(allOf(hasName(Name), isDefinition(),
+ unless(hasVisibilityAttr())))
----------------
Rather than a for loop, you can pass the list to `hasAnyName()`.
================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:63
+void AddVisibilityCheck::check(const MatchFinder::MatchResult &Result) {
+ for (const auto &Name : Names) {
+ if (const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>(Name)) {
----------------
Once you drop the for loop, you can pick a single name to bind to -- the
`check()` function will be called once for each match anyway, so you don't need
an explicit loop checking all of the name.
================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:65-66
+ if (const auto *MatchedDecl = Result.Nodes.getNodeAs<FunctionDecl>(Name)) {
+ std::string VisAttr =
+ ("__attribute__((visibility(\"" + Visibility + "\")))\n");
+ diag(MatchedDecl->getLocStart(),
----------------
You should use a `Twine` to construct this in place.
================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:68
+ diag(MatchedDecl->getLocStart(),
+ "visibility attribute not set for specified function")
+ << MatchedDecl
----------------
How about: "function expected to be annotated with '%0' visibility"
I'm mostly worried about the case where the function has a visibility
attribute, but it has the *wrong* visibility specified. The current wording
would be confusing in that circumstance. Or is that not a scenario you care
about, just that *any* visibility is specified on the function?
================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.h:21
+/// Finds functions given a list of names and suggests a fix to add
+/// an _`_attribute__((visibility("VISIBILITY")))` to their definitions,
+/// if they do not already have a visibility attribute.
----------------
The backtick is in the wrong place.
https://reviews.llvm.org/D43392
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
