sammccall 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; }
+
----------------
aaron.ballman wrote:
> 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.
> 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.
Thanks! John, please let me know if you see this causing a problem - I'm sure
we can find a solution but it's easier to find a good one if we know the
requirements :-)
(This makes me curious: officially, APIs are unstable and Hyrum's law is
Hyrum's problem. In practice, we certainly have the idea that if people
actually use something out-of-tree, it shouldn't be dropped without a high
level replacement. I haven't seen this written down, maybe it's just common
sense).
> 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.
Yeah, it's not as nice and declarative but it's not common as far as we know.
> 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.
Sounds good, I'll add that to this patch.
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
