The changes are for better diagnostic/error-messages. The error message of Clang, MSVC, and GCC were compared and MSVC gives more detailed error message so that is used now.
Fixes #64843
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
Test case updates are missing due to which 7 test cases are failing on pre-merge check - https://buildkite.com/llvm-project/premerge-checks/builds/172995#018a2776-1461-4f98-b12d-bd0521352d50/6-14972.
Comment Actions
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
Comment Actions
Thanks, I think we also want a note similar to MSVC diagnostic:
<source>(6): note: see reference to function 'Derived::Derived(void)'
Comment Actions
Just address @rsmith's comment, I think after that we are fine to commit this review.
+ "%sub{select_special_member_kind}0 cannot be 'constexpr' in a class or struct with virtual base classes">;
Please don't say "class or struct" here. Either just say "class" (a struct is a kind of class) or actually figure out which one it is and say that.
| clang/lib/Sema/SemaDeclCXX.cpp | ||
|---|---|---|
| 7824–7825 | 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 SDo you have some code examples where this note helps clarify in ways I'm not seeing from the test coverage? | |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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">;
| clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
|---|---|---|
| 9456–9458 | here is the error wording change | |
Comment Actions
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).
| clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
|---|---|---|
| 9456–9458 | Changes the wording to be singular instead of plural. | |
| 9457–9458 | Let's drop this note entirely (more on this below). | |
| clang/lib/Sema/SemaDeclCXX.cpp | ||
| 7824–7825 | 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. | |
Comment Actions
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.Comment Actions
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).
Comment Actions
@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
Comment Actions
Sorry about the delayed response -- the issue is that you've modified the diagnostic wording but not the logic for when we emit the diagnostic. I think you will need to end up modifying defaultedSpecialMemberIsConstexpr() to return information about *why* the member is not constexpr so that you can select between various diagnostic messages. e.g., you're implementing this FIXME: https://github.com/llvm/llvm-project/blob/c154ba8abeb6f59f85a9bb6fdf7bd79ad0d8c05c/clang/lib/Sema/SemaDeclCXX.cpp#L7817 This may also require you to add more diagnostics/%select uses because there are multiple reasons it could have failed. Does that make sense?
Comment Actions
Code:
struct Base {
constexpr Base() = default;
};
struct Derived : virtual Base {
constexpr Derived() = default;
};
struct NoCopyMove {
constexpr NoCopyMove() {}
NoCopyMove(const NoCopyMove&);
NoCopyMove(NoCopyMove&&);
};
struct S2 {
constexpr S2() = default;
constexpr S2(const S2&) = default;
constexpr S2(S2&&) = default;
NoCopyMove ncm;
};Error Generated
../../llvm-test/x.cpp:5:3: error: default constructor cannot be 'constexpr' in a class with virtual base class
5 | constexpr Derived() = default;
| ^
../../llvm-test/x.cpp:4:18: note: virtual base class declared here
4 | struct Derived : virtual Base {
| ^
../../llvm-test/x.cpp:15:3: error: defaulted definition of copy constructor is not constexpr
15 | constexpr S2(const S2&) = default;
| ^
../../llvm-test/x.cpp:16:3: error: defaulted definition of move constructor is not constexpr
16 | constexpr S2(S2&&) = default;
| ^
3 errors generated.@aaron.ballman do you mean like this? This now generates different diagnostics depending on whether a virtual base class is present or not
Comment Actions
@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
Comment Actions
Yes, this is along the lines of what I was thinking.
That sounds like a reasonable direction to me!
Comment Actions
@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?
Comment Actions
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.
| clang/docs/ReleaseNotes.rst | ||
|---|---|---|
| 216–217 | ||
| clang/test/CXX/dcl.decl/dcl.fct.def/dcl.fct.def.default/p2.cpp | ||
| 26–33 ↗ | (On Diff #557507) | I think the test coverage should live in clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp as that's where the wording is in the standard for this restriction (https://eel.is/c++draft/dcl.constexpr). |
Comment Actions
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.
Comment Actions
I don't have commit rights. You can use:
name: Nouman Amir
email: noumanamir453@gmail.com