[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:23-25
+  // if (const auto *D = Node.getDefinition()) {
+  //   return D->getExplicitVisibility(NamedDecl::VisibilityForType) == V;
+  // }
----------------
Can remove these comments.


================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:52-53
+  Finder->addMatcher(
+      functionDecl(allOf(hasAnyName(SmallVector<StringRef, 5>(Names.begin(),
+                                                              Names.end())),
+                         isDefinition(), unless(hasVisibilityAttr(V))))
----------------
Can you use `makeArrayRef()` rather than using a `SmallVector` that allocates?


================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:62
+          Result.Nodes.getNodeAs<FunctionDecl>("no-visibility")) {
+  	const Attr *A = D->getAttr<VisibilityAttr>();
+  	if (A && !A->isInherited() && A->getLocation().isValid())
----------------
`const auto *`


================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:66
+	         "%0 visibility attribute not set for specified function")
+	        << VisAttr << D
+	        << FixItHint::CreateReplacement(A->getRange(),
----------------
Why are you passing `D` as well?


================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:72
+	         "no explicit visibility attribute set for specified function")
+	        << VisAttr << D
+	        << FixItHint::CreateInsertion(D->getLocStart(),
----------------
Why are you passing in both `VisAttr` and `D`?


================
Comment at: test/clang-tidy/fuchsia-add-visibility.cpp:25
+// CHECK-MESSAGES: [[@LINE-2]]:1: warning: default visibility attribute not set for specified function
+// CHECK-FIXES: __attribute__((visibility("default")))
----------------
Should this test case diagnose, assuming `foo` is in the list of functions to check?
```
__attribute__((visibility("default"))) void foo(int);
void foo(double); // Diagnose?
```
How about this case?
```
template <typename Ty>
__attribute__((visibility("default"))) void foo(Ty);

template <>
void foo<int>(int); // Diagnose?
```


https://reviews.llvm.org/D43392