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

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added inline comments.


================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:26
+
+// AST_MATCHER(FunctionDecl, isInHeaderFile) {
+//   return Node.getExplicitVisibility(NamedDecl::VisibilityForType);
----------------
What are these comments doing here?


================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:49
+    Vis = DefaultVisibility;
+  else
+  	llvm::errs() << "Invalid visibliity attribute: " << VisAttr << "\n";
----------------
What about internal?


================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:64
+                                                              Names.end())),
+                         unless(hasVisibilityAttr(Vis))))
+          .bind("no-visibility"),
----------------
Something in the list that simply has explicit visibility should pass I think. For instance saying you have a blacklist of symbols instead of a whitelist. Sometimes internal or hidden might be used but we might want to add "hidden" by default. So "Vis" might be DefaultVisibility but we don't want to raise an error/change something labeled with internal visibility to hidden.


https://reviews.llvm.org/D43392