aaron.ballman added a comment. In D158540#4628904 <https://reviews.llvm.org/D158540#4628904>, @NoumanAmir657 wrote:
> In D158540#4628860 <https://reviews.llvm.org/D158540#4628860>, @aaron.ballman > wrote: > >> In D158540#4628629 <https://reviews.llvm.org/D158540#4628629>, >> @NoumanAmir657 wrote: >> >>> In D158540#4628310 <https://reviews.llvm.org/D158540#4628310>, >>> @aaron.ballman wrote: >>> >>>> In D158540#4628265 <https://reviews.llvm.org/D158540#4628265>, >>>> @NoumanAmir657 wrote: >>>> >>>>> No, I don't have code examples that showcase the importance of the note. >>>>> As you said the class context would be obvious whenever we run into this >>>>> error. >>>>> The test files also don't show where the note would be helpful. >>>> >>>> The crux of https://github.com/llvm/llvm-project/issues/64843 is about the >>>> error diagnostic, and that logic hasn't been changed in this patch -- are >>>> there other changes that are missing from the patch? The text of the tests >>>> shows that the error diagnostic behavior should have changed as well, but >>>> I'm not seeing the functional changes to make that happen. >>> >>> Can you elaborate further? I did not understand what you mean by functional >>> changes. According to my knowledge, I don't think anything else is missing >>> from the patch. >> >> The only changes I see in the review are the addition of >> `note_incorrect_defaulted_constexpr` and emitting that note. However, I see >> test cases changing from: >> >> constexpr W() __constant = default; // expected-error {{defaulted >> definition of default constructor is not constexpr}} >> >> to >> >> constexpr W() __constant = default; // expected-error {{default >> constructor cannot be 'constexpr' in a class with virtual base classes}} >> expected-note {{see reference to function 'W' in 'W' class}} >> >> where the error wording is now different, but I don't see any code changes >> to the compiler for the error wording. > > I changed the error wording in the file DiagnosticSemaKinds.td > > "%sub{select_special_member_kind}0 cannot be 'constexpr' in a class with > virtual base classes">; Okay, *now* I am seeing some changes there .. that's really strange, because my previous viewing only showed the *note* changing. I'm very sorry for the confusing noise; it could be that Phab is a bit flaky (it's considerably slower than it usually is). ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9396 def err_incorrect_defaulted_constexpr : Error< - "defaulted definition of %sub{select_special_member_kind}0 " - "is not constexpr">; + "%sub{select_special_member_kind}0 cannot be 'constexpr' in a class with virtual base classes">; +def note_incorrect_defaulted_constexpr : Note< ---------------- NoumanAmir657 wrote: > here is the error wording change Changes the wording to be singular instead of plural. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9397-9398 + "%sub{select_special_member_kind}0 cannot be 'constexpr' in a class with virtual base classes">; +def note_incorrect_defaulted_constexpr : Note< + "see reference to function %1 in %0 class">; def err_incorrect_defaulted_consteval : Error< ---------------- Let's drop this note entirely (more on this below). ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:7808-7809 + if (!MD->isConsteval()) { + Diag(MD->getBeginLoc(), diag::note_incorrect_defaulted_constexpr) + << RD << MD; + } ---------------- aaron.ballman wrote: > I'm not certain I understand how this note helps users -- the note is always > attached to the instance method being defaulted, so it will always appear > where the class context is obvious. e.g., > ``` > struct S { > constexpr S() = default; // Can quickly tell we're in class S > constexpr S(const S&); > }; > > constexpr S::S(const S&) = default; // Can still quickly tell we're in class S > ``` > Do you have some code examples where this note helps clarify in ways I'm not > seeing from the test coverage? Instead of issuing this note, I think we should issue `diag::note_constexpr_virtual_base_here` and specify the location of a virtual base class. This way, the user sees the diagnostic saying "can't do this because of a virtual base class" and the note directs the user back to the problem. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158540/new/ https://reviews.llvm.org/D158540 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
