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

[Scott Linder via Phabricator via cfe-commits]

scott.linder marked 3 inline comments as done.
scott.linder added inline comments.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:2621
+    if (!ND->isExternallyVisible()) {
+      S.Diag(AL.getRange().getBegin(), diag::warn_attribute_ignored) << AL;
+      return;
----------------
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
```


================
Comment at: lib/Sema/SemaDeclAttr.cpp:2622
+      S.Diag(AL.getRange().getBegin(), diag::warn_attribute_ignored) << AL;
+      return;
+    }
----------------
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.


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

https://reviews.llvm.org/D61097