[PATCH] D103159: [Clang] Enable __has_feature(coverage_sanitizer)

Marco Elver via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed May 26 11:19:06 PDT 2021


melver added inline comments.


================
Comment at: clang/include/clang/Basic/Features.def:52
         LangOpts.Sanitize.hasOneOf(SanitizerKind::Undefined))
+FEATURE(coverage_sanitizer, LangOpts.SanitizeCoverage)
 FEATURE(assume_nonnull, true)
----------------
ojeda wrote:
> melver wrote:
> > aaron.ballman wrote:
> > > I think this is likely fine, but wanted to call it out explicitly in case others had opinions.
> > > 
> > > `FEATURE` is supposed to be used for standard features and `EXTENSION` used for Clang extensions. This is an extension, not a standard feature, so it's wrong in that way. However, it's following the same pattern as the other sanitizers which is consistent. I think consistently wrong is better than inconsistently right for this case, but I have no idea how others feel.
> > Yes, you are correct of course, and I was pondering the same thing.
> > 
> > In the end I'd like all sanitizers be queryable via `__has_feature()` and not have this be the odd one out requiring `__has_extension()` as that's also going to lead to confusion/errors on the user side. 
> Perhaps add both, deprecate `__has_feature()` for non-standard features like these ones, and remove them after a couple releases? :)
> 
> Regardless of the way, //any// is better than a version check, so thanks!
I think realistically we have to pick one, and that's the one we keep for all eternity. :-)

Because if we deprecate/remove something, some codebases would require version checks, which is a non-starter again. Not every project is on top of what their compilers deprecates/removes. (And, unlike the Linux kernel, some codebases just never upgrade their compiler requirements, but still expect newer compilers to work.)

So if we want consistency with other sanitizers, it has to be `__has_feature()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103159



More information about the cfe-commits mailing list