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