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

[Roy Jacobson via Phabricator via cfe-commits]

royjacobson marked 9 inline comments as done and 2 inline comments as done.
royjacobson 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">;
----------------
aaron.ballman wrote:
> 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?
Sounds good to me, updated it.


================
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>;
----------------
aaron.ballman wrote:
> 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`?
Took Aaron's version at the end.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5400-5401
+                                            SourceLocation KWLoc) {
+  if (!S.getLangOpts().CPlusPlus11)
+    return;
+
----------------
aaron.ballman wrote:
> I think we should always warn on these, not just in C++11.
I'm not convinced we should. My reasoning is that we need a pretty good reason to start issuing warnings for 20 years old code. The usage of those builtins with deleted functions after C++11 is pretty broken which is a pretty good reason, but for earlier language versions they work 'fine' and if people want to use C++03 I prefer leaving them at peace :)

People on C++03 are also probably using pretty old versions of libstdc++ and/or boost type_traits, so this could have some impact.

WDYT?



================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5426-5428
+    case UTT_HasTrivialDefaultConstructor:
+      Replacement = TT_IsTriviallyConstructible;
+      break;
----------------
aaron.ballman wrote:
> 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.
It's the `__has_trivial_constructor` builtin that unfortunately has a different enum name (it's named after the internal CXXRecordDecl it calls, I think).


================
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:
----------------
aaron.ballman wrote:
> 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.
It's used in libstdc++'s type_traits and in boost's type_traits (but we will start warning on boost's type_traits anyway). So it's even if by default people are not going to see it I think it's used widely enough to be problematic and we shouldn't warn on this for now.

I added the libstdc++ part to the comment as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129170