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

[Roy Jacobson via Phabricator via cfe-commits]

royjacobson added inline comments.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:5400-5401
+                                            SourceLocation KWLoc) {
+  if (!S.getLangOpts().CPlusPlus11)
+    return;
+
----------------
aaron.ballman wrote:
> royjacobson wrote:
> > aaron.ballman wrote:
> > > erichkeane wrote:
> > > > royjacobson wrote:
> > > > > erichkeane wrote:
> > > > > > royjacobson wrote:
> > > > > > > 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?
> > > > > > > 
> > > > > > warnings don't get emitted for code in header files, so at least that part isn't a concern.  
> > > > > Any header files, or just system headers?
> > > > Sorry, yes, Phab is a mess on a cell phone... in things included as System Headers.
> > > Agreed with Erich -- warnings in system headers (but not regular headers) are silenced by default, you need to pass `-Wsystem-headers` to enable them.
> > To clarify my position, I think about those builtins as an unofficial part of the C++03 standard and I think we should support them as long as we support C++03.
> > 
> > Do you think that's reasonable?
> > 
> > I agree we should update the documentation in this case.
> I still don't see the benefit of undeprecating these in C++03. The replacement interfaces are available for use in C++03 mode, so there's nothing that prevents a user from being pushed to use the supported interfaces instead, right? To me, the older interfaces were not clean/correct and the replacement interfaces are the ones that we want people to use, and that's language mode agnostic.
I think they're 
1. Clean enough under C++03 semantics - without the extra complications of move semantics, defaulted/deleted functions etc.
2. Potentially pretty widely used in code that needs C++03, though I have no idea how to check that.

But you convinced me that it's probably not that of a big deal to add those warnings anyway, so let's add them and think about actual deprecation in a few years :)



================
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:
> royjacobson wrote:
> > 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.
> My feeling is: if it's deprecated and we don't warn on it, it's not actually deprecated. I'd rather see us warn uniformly on all the deprecated interfaces. System headers are free to keep using these interfaces (system headers are long lived), but any user code using this needs some sort of notification that they're using something we don't want them to use, or we will never be able to get rid of these interfaces (at which point, we should undeprecate them because we need to keep them working).
Ok, added it to the warnings.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129170