[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: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