aaron.ballman added a subscriber: john.brawn.
aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Sema/ParsedAttr.h:96
+ /// Check if this attribute is allowed by the language we are compiling.
+ virtual bool acceptsLangOpts(const LangOptions &LO) const { return true; }
+
----------------
sammccall wrote:
> aaron.ballman wrote:
> > Plugin attributes inherit from `ParsedAttrInfo`, not `ParsedAttr`, so one
> > downside to this change is that plugin authors no longer have a way to
> > diagnose language options with a custom diagnostic; all they can get is
> > "attribute ignored".
> >
> > Perhaps another approach is to add an output parameter so the overrider can
> > signify whether the language options are valid or not (because it's
> > plausible that the plugin wants to diagnose the language options but
> > they're still valid enough that the attribute should be accepted)?
> >
> > Plugin attributes inherit from ParsedAttrInfo, not ParsedAttr, so one
> > downside to this change is that plugin authors no longer have a way to
> > diagnose language options with a custom diagnostic; all they can get is
> > "attribute ignored".
>
> This is less flexible, indeed. What's not clear to me is:
> - do we know of any real plugins where the ability to use a custom
> diagnostic here is important? Hypothetical flexibility may not be worth
> keeping.
> - if custom diagnostics are needed, can they be emitted when the attribute
> is handled instead?
> - why we'd need this flexibility for LangOpts but not Target (existsInTarget)
>
> > Perhaps another approach is to add an output parameter so the overrider can
> > signify whether the language options are valid or not (because it's
> > plausible that the plugin wants to diagnose the language options but
> > they're still valid enough that the attribute should be accepted)?
>
> diagLangOpts already returns bool.
>
> The blocking issue with the diagLangOpts signature for code-completion
> purposes isn't actually the diagnostics (completion suppresses them), it's
> rather that you have to construct a ParsedAttr in order to emit them, which
> isn't easy to do "out of thin air".
> do we know of any real plugins where the ability to use a custom diagnostic
> here is important? Hypothetical flexibility may not be worth keeping.
We don't know how plugins are being used and we have to assume Hryum's Law
bites us just as much as anyone else. I'm fine potentially breaking plugins so
long as we're still leaving them with the ability to do what they were
previously doing. However, @john.brawn (the attribute plugin author) might know
more here on the usage front.
> if custom diagnostics are needed, can they be emitted when the attribute is
> handled instead?
Good question! I think that might work -- they get access to `Sema` and so they
can check the language options and produce diagnostics from there. It might
mean slight behavioral changes (if we used to bail early on a lang opt
mismatch, it means we might get different diagnostics by the delay until the
attribute is handled), but I think those are reasonable.
> why we'd need this flexibility for LangOpts but not Target (existsInTarget)
Targets are a bit different -- you're in one target and that's it (so you
either support the attribute for that target or not), but language options can
have interactions between them (attribute may require something like CUDA to be
enabled and it may also require some other CUDA-specific option to be enabled,
and it wants to produce a nice diagnostic about that rather than just saying
the attribute is ignored).
> The blocking issue with the diagLangOpts signature for code-completion
> purposes isn't actually the diagnostics (completion suppresses them), it's
> rather that you have to construct a ParsedAttr in order to emit them, which
> isn't easy to do "out of thin air".
Ahh, I had missed that.
Given that the user can still do custom diagnostics when handling the attribute
itself, I think this change is reasonable as-is. However, I think you should
add a mention to the release notes that 1) the interface changed for plugin
authors, and 2) the new approach to custom diagnostics for language options is
to diagnose them when handling the attribute.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107836/new/
https://reviews.llvm.org/D107836
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
