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