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

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.


================
Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:68
+      diag(MatchedDecl->getLocStart(),
+           "visibility attribute not set for specified function")
+          << MatchedDecl
----------------
aaron.ballman wrote:
> jakehehrlich wrote:
> > aaron.ballman wrote:
> > > 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?
> > The use case for this check is forcing code bases to carefully control what symbols are exported rather than just exporting everything. So if someone took the time to explicitly set the visibility of one of these symbols we care about then we should assume they knew what they were doing.
> > 
> > The specific use case I care about for this check is using -fvisibility=hidden and then checking to make sure a certain curated list of symbols has explicit default visibility.
> Ah, so a mismatch is unimportant, that's good to know. How about: "function expected to be annotated with the 'visibility' attribute"?
So in implementing a way to handle #pragma and command line directives, it became clear that the more comprehensive check (that replaces existing attrs) was a trivial implementation on top of that, so I put it in. If we just want to ignore those cases, I can take out the conditional path.


https://reviews.llvm.org/D43392