shafik added inline comments.
================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9410-9421 +def ext_incorrect_defaulted_comparison_constexpr : Extension< "defaulted definition of %select{%sub{select_defaulted_comparison_kind}1|" "three-way comparison operator}0 " - "cannot be declared %select{constexpr|consteval}2 because " + "declared %select{constexpr|consteval}2 but " "%select{it|the corresponding implicit 'operator=='}0 " - "invokes a non-constexpr comparison function">; + "invokes a non-constexpr comparison function are a C++2b extension">, InGroup<CXX2bDefCompCallsNonConstexpr>; +def warn_cxx2b_incorrect_defaulted_comparison_constexpr : Warning< ---------------- rsmith wrote: > The grammar of these diagnostics looks a bit peculiar. Suggested an > alternative, but it's still a bit awkward ("...but for which..."), maybe you > can find something better. @aaron.ballman any suggestions for less awkward wording here? I don't think what there is here now is too bad but asking for a another opinion. ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8808-8809 if (FD->isConstexpr()) { if (CheckConstexprReturnType(*this, FD, CheckConstexprKind::Diagnose) && CheckConstexprParameterTypes(*this, FD, CheckConstexprKind::Diagnose) && !Info.Constexpr) { ---------------- rsmith wrote: > We'll also be getting diagnostics here if the parameter and return types are > not literal types. You can pass `...::CheckValid` instead of `...::Diagnose` > to find out if there would be an error, but we should probably instead make > the `CheckConstexpr...` functions produce warnings (`ExtWarn` prior to C++23) > rather than errors in general. That's a much larger change, though -- there's > probably half a dozen diagnostics in the `CheckConstexpr*` functions that > will need to be updated to properly support P2448R2. If you just want to > implement this one part of P2448 for now and leave the rest for a later > change, that seems fine to me. I don't think I have the bandwidth for a larger change but I can file a bug and assign myself to it and I will put in my work queue. ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8813 + (getLangOpts().CPlusPlus2b ? diag::warn_cxx2b_incorrect_defaulted_comparison_constexpr : + diag::ext_incorrect_defaulted_comparison_constexpr )) << FD->isImplicit() << (int)DCK << FD->isConsteval(); ---------------- rsmith wrote: > Don't need the parens around this. This layout is a little unusual, what does > clang-format do here? My bad, I forgot to apply clang-format. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146090/new/ https://reviews.llvm.org/D146090 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits