In D158540#4652792, @aaron.ballman wrote:

The only thing left are the comments on the release note, but I can fix that up when landing if you need me to commit on your behalf. If so, what name and email address would you like me to use for patch attribution? Otherwise, if you have commit privileges, then you can fix up the release note before landing the changes.

In D158540#4652051, @aaron.ballman wrote:

In D158540#4651859, @NoumanAmir657 wrote:

@aaron.ballman
Do I need to make changes other than this? The virtual base diagnostic doesn't have a test case in files that would generate it. Should I add the above example as the test case?

You'll need to add a test case to demonstrate the changes, and the example you had above would work well for the tests. You should also add a release note to clang/docs/ReleaseNotes.rst so users know about the improvement. But I think that should basically be all that's left to do here.

@aaron.ballman
Do I need to make changes other than this? The virtual base diagnostic doesn't have a test case in files that would generate it. Should I add the above example as the test case?

@aaron.ballman
why the member is not an constexpr can be seen from getNumVBases(). The 'defaultedSpecialMemberIsConstexpr' returns false whenever getVNumBases is true for this so we can use that to identify when to give which error diagnostic.
Can you verify whether this would work or is there any problem with this. I haven't uploaded the diff yet

Code:

struct Base {
 constexpr Base() = default;
};
struct Derived : virtual Base {
  constexpr Derived() = default;
};

In D158540#4632457, @NoumanAmir657 wrote:

@aaron.ballman
This error gets generated on test cases even when a struct/class as no virtual base class.
see this example on here: example

Is this right behaviour? The note for this should not be generated since this does not have a virtual base class.

The example above is from a test case file in clang. The error message is the one which was supposed to be improved for this patch.

Can you clarify this

@aaron.ballman
This error gets generated on test cases even when a struct/class as no virtual base class.
see this example on here: example

In D158540#4632240, @aaron.ballman wrote:

In D158540#4629042, @NoumanAmir657 wrote:

Do you want the note to be like this:

../llvm-test/x.cpp:5:1: note: virtual base class declared here
    5 | struct Derived : virtual Base {
      | ^
1 error generated.

Close! I was thinking more like:

../llvm-test/x.cpp:5:1: note: virtual base class declared here
    5 | struct Derived : virtual Base {
      |                          ^
1 error generated.

(so the location points to the base class and not the derived class).

In D158540#4628958, @aaron.ballman wrote:

In D158540#4628904, @NoumanAmir657 wrote:

In D158540#4628860, @aaron.ballman wrote:

In D158540#4628629, @NoumanAmir657 wrote:

In D158540#4628310, @aaron.ballman wrote:

In 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).

In D158540#4628958, @aaron.ballman wrote:

In D158540#4628904, @NoumanAmir657 wrote:

In D158540#4628860, @aaron.ballman wrote:

In D158540#4628629, @NoumanAmir657 wrote:

In D158540#4628310, @aaron.ballman wrote:

In 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).

In D158540#4628860, @aaron.ballman wrote:

In D158540#4628629, @NoumanAmir657 wrote:

In D158540#4628310, @aaron.ballman wrote:

In 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.

In D158540#4628310, @aaron.ballman wrote:

In 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.

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.

@xgupta the build is successful now. Earlier it failed due to format issues.

@xgupta It passed the test cases now

I figured it out and changed the test files.

How am I supposed to update tests? I am new to this. I get the files on which the tests fail but how do I update them

The pre-commit checks passed now when I built again. Looking into test for this now