This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Emit warning for visibility attribute on internal-linkage declaration
ClosedPublic

Authored by scott.linder on Apr 24 2019, 3:00 PM.

Diff Detail

Event Timeline

scott.linder created this revision.Apr 24 2019, 3:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2019, 3:00 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
lib/Sema/SemaDeclAttr.cpp
2619

const auto *ND please.

2621

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.

2622

We shouldn't early return here (it's not an error, just a warning), so that the other diagnostics can also trigger if needed.

scott.linder marked 3 inline comments as done.Apr 26 2019, 8:02 AM
scott.linder added inline comments.
lib/Sema/SemaDeclAttr.cpp
2621

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
2622

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.

Clean up use of auto

aaron.ballman added inline comments.May 1 2019, 3:44 AM
lib/Sema/SemaDeclAttr.cpp
2621

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.

2622

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!

Use distinct sema diagnostic and do not return early.

I am going to bump DIAG_SIZE_SEMA in another patch, as it seems like we have hit the current limit.

aaron.ballman accepted this revision.May 1 2019, 12:52 PM

LGTM, thank you!

This revision is now accepted and ready to land.May 1 2019, 12:52 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2019, 12:02 PM

Richard Smith on cfe-dev pointed out some cases where this patch is incorrect, stemming from trying to calculate the linkage too early; the warning will either have to work without the use of isExternallyVisible or will have to be emitted later. This was reverted in r359858 for now.