[PATCH] D107836: [Attributes]: refactor to expose ParsedAttrInfo::acceptsLangOpts. NFC

[Sam McCall via Phabricator via cfe-commits]

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