[PATCH] D146420: Document the Clang policies on claiming support for a feature

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 5 inline comments as done.
aaron.ballman added inline comments.


================
Comment at: clang/docs/InternalsManual.rst:3285
+  * Are there known issues with the feature that reject valid code?
+  * Are there known issues that fail to reject invalid code?
+  * Are there known crashes, failed assertions, or miscompilations?
----------------
cor3ntin wrote:
> any way to rephrase that, "that cause clamg to reject" maybe?
Sure, we can go with "accept invalid code" instead of "fail to reject invalid".


================
Comment at: clang/docs/InternalsManual.rst:3289
+
+If the answer to any of these is "yes", you should either not define the
+feature test macro or have very strong rationale for why the issues should not
----------------
erichkeane wrote:
> you -> we?  Rest of the doc is in 3rd person, this is in 2nd?
Reworded to remove the pronoun entirely.


================
Comment at: clang/docs/InternalsManual.rst:3292
+prevent defining it. Note, it is acceptable to define the feature test macro on
+a per-target basis if needed.
+
----------------
cor3ntin wrote:
> Is that desirable?
> If i have a build system checking for support through feature macros, I'm not sure they do that on a target per target basis.
Maybe it's not desirable, I'm happy to hear arguments either way. The reason why I think this is reasonable is because we'll sometimes have a feature that works just fine everywhere EXCEPT a target (like coroutines work everywhere but have known miscompilation issues on 32-bit Windows). It seems somewhat unfortunate to not claim support anywhere because it's not supported on all targets -- some users likely never target 32-bit Windows in the first place.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146420/new/

https://reviews.llvm.org/D146420