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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
