[PATCH] D129170: [Sema] Add deprecation warnings for some compiler provided __has_* type traits

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:190
     : DiagGroup<"deprecated-dynamic-exception-spec">;
+def DeprecatedHasBuiltins : DiagGroup<"deprecated-has-builtins">;
 def DeprecatedImplementations :DiagGroup<"deprecated-implementations">;
----------------
I wonder if we want to rename this to `deprecated-builtins` so it applies to any builtin we elect to deprecate, not just ones that start with `has`. WDYT?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5561
+def warn_deprecated_has_builtins : Warning<
+  "the %0 compiler builtin is deprecated from C++11 onwards. Use %1 instead.">,
+  InGroup<DeprecatedHasBuiltins>;
----------------
erichkeane wrote:
> cor3ntin wrote:
> > Should we say something like "and will be removed in the future"?
> > 
> > "%0 is deprecated from C++11 onward. Use %1 instead." might be sufficient
> > 
> > 
> Diagnostics arent sentences. Also, we wouldn't say "C++11 onward", we can just say C++11. I might suggest:
> 
> `builtin %0 is deprecated in C++11, use %1 instead`
> 
> BUT @aaron.ballman always does great at this level of bikeshedding.
> 
I think we might want to rename this to `warn_deprecated_builtin` and make it slightly more general. I think we want to drop the mention of C++11 because the documentation says these are deprecated (not deprecated in a specific version) and the replacement APIs are all available pre C++11 anyway (at least in Clang). How about: `builtin %0 is deprecated; use %1 instead`?


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5400-5401
+                                            SourceLocation KWLoc) {
+  if (!S.getLangOpts().CPlusPlus11)
+    return;
+
----------------
I think we should always warn on these, not just in C++11.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5426-5428
+    case UTT_HasTrivialDefaultConstructor:
+      Replacement = TT_IsTriviallyConstructible;
+      break;
----------------
This one is not documented as being deprecated (or documented at all). I think it's reasonable to deprecate it, but it should probably be documented in the language extensions page.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5433
+    // FIXME: GCC don't implement __is_trivially_destructible: Warning for this
+    //   might be too noisy for now.
+    // case UTT_HasTrivialDestructor:
----------------
I'd like to know how plausible that "might" is -- Clang flags it as being deprecated, so it seems very reasonable to diagnose it as such. These diagnostics won't be triggered from system headers by default anyway, so I'm not certain if this will be too noisy in practice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129170