aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
In D103159#2784845 <https://reviews.llvm.org/D103159#2784845>, @melver wrote: > Ping. FWIW, the usual practice is to ping after no activity on the review for about a week. That said, LGTM! ================ 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: > > 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()`. > Agreed, any friction on updates is bad for users and will annoy someone > somewhere. > > Having said that, any serious project migrating to a new toolchain needs to > revalidate regardless. And, after all, these are non-standard features. So in > practice I do not think it would matter too much if the deprecation notice is > long enough (several years). > > But I may be saying something completely stupid, since I do not even know if > Clang promises forever-stability for options, like `rustc` does. I agree with @melver that it'd be worse to deprecate the feature testing macro. Then you have to use compiler version checks to decide which way to spell the feature testing macro, which largely defeats the entire purpose of having feature testing macros in the first place. I think we're basically stuck with a policy that all the sanitizers can be tested as features rather than extensions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103159/new/ https://reviews.llvm.org/D103159 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
