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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
