[PATCH] D158540: Improve error message for constexpr constructors of virtual base classes

[Aaron Ballman via Phabricator via cfe-commits]

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