-
Notifications
You must be signed in to change notification settings - Fork 14.5k
Add Clang attribute to ensure that fields are initialized explicitly #102040
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: None (higher-performance) ChangesThis is a new Clang-specific attribute to ensure that field initializations are performed explicitly. For example, if we have
|
@AaronBallman I think this is worth of an RFC, WDYT? |
Thank you for this!
Yes, this should definitely get an RFC. Some things worth discussing in the RFC:
(I'm sure there are other questions, but basically, it's good to have a big-picture understanding of why a particular design is the way you think we should go.) |
f09b427
to
5e03c06
Compare
5e03c06
to
9850a8b
Compare
I updated the PR to change the attribute name from @AaronBallman are we good to move forward? |
9850a8b
to
471b41a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope folks are ok with me chiming in as a reviewer for this.
I've left quite a few comments in the RFC and is also supportive of landing this change and happy to invest into supporting it going forward inside our team.
002027b
to
1695451
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
9833f57
to
dc56548
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are some good parallel disscussions happening in the RFC, but despite their outcomes, we could probably update the PR to capture current behavior in those interesting cases.
I left a few comments along those lines, PTAL.
4319585
to
9335d80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment about VerifyOnly
and duplicate diagnostics.
The rest are small NITs.
clang/test/SemaCXX/uninitialized.cpp
Outdated
C a; // expected-warning {{not explicitly initialized}} | ||
(void)a; | ||
#endif | ||
D b{.f2 = 1}; // expected-warning {{'x' is not explicitly initialized}} expected-warning {{'q' is not explicitly initialized}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an idea for future improvements: we could also collect all unitialized fields and emit a single diagnostic that lists them all (with notes to the locations of the fields).
However, I think this is good enough for the first version, I don't necessarily feel we should do it right away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's more complicated than I can invest in right now, it's something we can do in the future though.
@AaronBallman @cor3ntin I believe we are getting close to finalizing this PR. There was some discussion here and in the RFC, but I don't think there was explicit approval (or objection) to land this, so I wanted to clarify this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the rfc has reached its conclusion yet, and consensus has not been called (for example, i still need to think about whether my questions were addressed) so we should wait for the RFC process before continuing with that PR.
Thanks
Thanks for explicitly calling this out. There were no replies from you on the RFC for some time, so it was unclear whether there is anything left. We will be waiting for your feedback on Discourse. |
Usually the process requires someone like Aaron to call consensus either
way. we are aware that establishing consensus is a bit nebulous at the
moment but it's something we are working on improving.
…On Fri, Sep 13, 2024, 16:03 Ilya Biryukov ***@***.***> wrote:
I don't think the rfc has reached its conclusion yet, and consensus has
not been called (for example, i still need to think about whether my
questions were addressed) so we should wait for the RFC process before
continuing with that PR.
Thanks
Thanks for explicitly calling this out. There were no replies from you on
the RFC for some time, so it was unclear whether there is anything left. We
will be waiting for your feedback on Discourse.
—
Reply to this email directly, view it on GitHub
<#102040 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKX7633PBO3XB4FJRFVRGTZWLWCNAVCNFSM6AAAAABMA3ZTVOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBZGA2DCMRWGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
9335d80
to
7ea9d3d
Compare
@AaronBallman Oh no worries at all. I'm actually struggling with something else -- apparently C++20 aggregate initialization with parentheses isn't handled correctly, and I'm struggling to get it to work. I added a bunch of test cases that all fail... any chance you know how these need to be handled? I tried adding
to the beginning of |
re the C++20 init: the code that handles this is here: llvm-project/clang/lib/Sema/SemaInit.cpp Line 5857 in 2d62daa
You will probably have to update it accordingly. I believe it would be ideal that this code was shared with llvm-project/clang/lib/Sema/SemaInit.cpp Line 769 in 2d62daa
it is easy to imagine that we would add that to list initializers as the most commonly used pattern and forget to add it for C++20 parenthesized initialization. |
Yup that's basically what I tried, but it seems to miss some cases and duplicate others. Hmm.. |
… it work in both C and C++
5ed7102
to
eddf80c
Compare
Looks like the tests finally all pass with my last fix. Could we merge? :) Happy new year! |
Hi @AaronBallman~ would you have a chance to do a final review? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for your patience on the review!
Fantastic, thank you! Could we merge it? |
heh, thanks for the poke, I didn't remember you didn't have commit access. I've merged now. |
Ah haha I see, thanks! Looking forward to getting it sometime, that would definitely be helpful :) |
FWIW, I think you're fine to ask for commit privs now: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access |
@AaronBallman I... just noticed that the name |
This is a new Clang-specific attribute to ensure that field initializations are performed explicitly.
For example, if we have
then the diagnostic would trigger if we do
B b{};
:This prevents callers from accidentally forgetting to initialize fields, particularly when new fields are added to the class.
Naming:
We are open to alternative names; we would just like their meanings to be clear. For example,
must_init
,requires_init
, etc. are some alternative suggestions that would be fine. However, we would like to avoid a name such asrequired
asmust_specify
, as their meanings might be potentially unclear or confusing (e.g., due to confusion withrequires
).Note:
I'm running into an issue with duplicated diagnostics (see lit tests) that I'm not sure how to properly resolve, but I suspect it revolves around
VerifyOnly
. If you know the proper fix please let me know.