[PATCH] D61097: [Sema] Emit warning for visibility attribute on internal-linkage declaration

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:2621
+    if (!ND->isExternallyVisible()) {
+      S.Diag(AL.getRange().getBegin(), diag::warn_attribute_ignored) << AL;
+      return;
----------------
scott.linder wrote:
> aaron.ballman wrote:
> > I think it would be more helpful for users if we introduced a new diagnostic here that explained why the attribute is being ignored, otherwise the diagnostic is somewhat indecipherable.
> Sure, I am amenable to anything here. GCC uses the same short message, but it does seem to indicate e.g. that the keyword `static` played some role by using additional source ranges. I don't know how we would go about that with the Sema/AST split? We could just go with a more explicit message. Do you have any thoughts on the wording?
> 
> ```
> warning: 'visibility' attribute ignored on non-external symbol
> ```
Your suggested diagnostic text seems pretty close to me, but how about: `'visibility' attribute is ignore on a non-external symbol` in the `IgnoredAttributes` group.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:2622
+      S.Diag(AL.getRange().getBegin(), diag::warn_attribute_ignored) << AL;
+      return;
+    }
----------------
scott.linder wrote:
> aaron.ballman wrote:
> > We shouldn't early return here (it's not an error, just a warning), so that the other diagnostics can also trigger if needed.
> Should I also fix the other early-return-on-warning's in the same function, e.g. the function begins with:
> 
> ```
> // Visibility attributes don't mean anything on a typedef.
> if (isa<TypedefNameDecl>(D)) {
>   S.Diag(AL.getRange().getBegin(), diag::warn_attribute_ignored) << AL;
>   return;
> }
> ```
> 
> I suppose the difference is that the attribute "can" be placed on the symbol in this case, but it is ignored just the same.
That might be worth fixing as well, but as a separate patch because it's also kind of unrelated to the functionality in this patch. Good catch!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61097/new/

https://reviews.llvm.org/D61097