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:
> 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".
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107836/new/
https://reviews.llvm.org/D107836
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
