[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:52-53
+  Finder->addMatcher(
+      functionDecl(allOf(hasAnyName(SmallVector<StringRef, 5>(Names.begin(),
+                                                              Names.end())),
+                         isDefinition(), unless(hasVisibilityAttr(V))))
----------------
juliehockett wrote:
> aaron.ballman wrote:
> > Can you use `makeArrayRef()` rather than using a `SmallVector` that allocates?
> Type-wise it gets funky -- `makeArrayRef()` creates an `ArrayRef<std::string>`, and the matcher wants a container of `StringRefs`. Is there a good way to do that without allocating?
I think there is, but it is orthogonal to your patch to make that many changes, so you can ignore my suggestion. I didn't see the type shenanigans there.


================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:40
+                                        HeaderFileExtensions, ',')) {
+    llvm::errs() << "Invalid header file extension: "
+                 << RawStringHeaderFileExtensions << "\n";
----------------
This works for now, but it would be nice to expose "driver diagnostics" for sanity checking clang-tidy configuration options. These diagnostics would not be required to pass in a source location, merely the diagnostic text. We already support thing kind of things for driver-generated diagnostics, but it's not exposed to clang-tidy yet.


================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:49
+    Vis = DefaultVisibility;
+  else
+  	llvm::errs() << "Invalid visibliity attribute: " << VisAttr << "\n";
----------------
jakehehrlich wrote:
> What about internal?
Might be a good use of `StringSwitch`.


================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:75
+	          Result.SourceManager->getExpansionLoc(D->getLocStart()))) {
+	    // unless that file is a header.
+	    if (!utils::isSpellingLocInHeaderFile(
----------------
Capitalization


https://reviews.llvm.org/D43392