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