Fix false negative when value initializing a field annotated with [[clang::require_field_initialization]] #124329
Conversation
…lang::require_field_initialization]]
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (higher-performance) ChangesIt turns out we weren't handling one case: the value-initialization of a field inside a struct. I'm not sure why this falls under Full diff: https://github.com/llvm/llvm-project/pull/124329.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index b95cbbf4222056..450edcb52ae15b 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -4573,7 +4573,9 @@ static void TryConstructorInitialization(Sema &S,
CXXConstructorDecl *CtorDecl = cast<CXXConstructorDecl>(Best->Function);
if (Result != OR_Deleted) {
- if (!IsListInit && Kind.getKind() == InitializationKind::IK_Default &&
+ if (!IsListInit &&
+ (Kind.getKind() == InitializationKind::IK_Default ||
+ Kind.getKind() == InitializationKind::IK_Direct) &&
DestRecordDecl != nullptr && DestRecordDecl->isAggregate() &&
DestRecordDecl->hasUninitializedExplicitInitFields()) {
S.Diag(Kind.getLocation(), diag::warn_field_requires_explicit_init)
diff --git a/clang/test/SemaCXX/uninitialized.cpp b/clang/test/SemaCXX/uninitialized.cpp
index 52d9897cf9be6e..7578b288d7b3fe 100644
--- a/clang/test/SemaCXX/uninitialized.cpp
+++ b/clang/test/SemaCXX/uninitialized.cpp
@@ -2,6 +2,8 @@
// RUN: %clang_cc1 -fsyntax-only -Wall -Wc++20-compat -Wuninitialized -Wno-unused-value -Wno-unused-lambda-capture -Wno-uninitialized-const-reference -std=c++1z -verify %s -fexperimental-new-constant-interpreter
// RUN: %clang_cc1 -fsyntax-only -Wall -Wc++20-compat -Wuninitialized -Wno-unused-value -Wno-unused-lambda-capture -Wno-uninitialized-const-reference -std=c++20 -verify %s
+void* operator new(__SIZE_TYPE__, void*);
+
// definitions for std::move
namespace std {
inline namespace foo {
@@ -1540,6 +1542,48 @@ void aggregate() {
};
};
+ struct Embed {
+ int embed1; // #FIELD_EMBED1
+ int embed2 [[clang::require_explicit_initialization]]; // #FIELD_EMBED2
+ };
+ struct EmbedDerived : Embed {};
+ struct F {
+ Embed f1;
+ // expected-warning@+1 {{field in 'Embed' requires explicit initialization but is not explicitly initialized}} expected-note@#FIELD_EMBED2 {{'embed2' declared here}}
+ explicit F(const char(&)[1]) : f1() {
+ // expected-warning@+1 {{field in 'Embed' requires explicit initialization but is not explicitly initialized}} expected-note@#FIELD_EMBED2 {{'embed2' declared here}}
+ ::new(static_cast<void*>(&f1)) decltype(f1);
+ // expected-warning@+1 {{field in 'Embed' requires explicit initialization but is not explicitly initialized}} expected-note@#FIELD_EMBED2 {{'embed2' declared here}}
+ ::new(static_cast<void*>(&f1)) decltype(f1)();
+#if __cplusplus >= 202002L
+ // expected-warning@+1 {{field 'embed2' requires explicit initialization but is not explicitly initialized}} expected-note@#FIELD_EMBED2 {{'embed2' declared here}}
+ ::new(static_cast<void*>(&f1)) decltype(f1)(1);
+#endif
+ // expected-warning@+1 {{field 'embed2' requires explicit initialization but is not explicitly initialized}} expected-note@#FIELD_EMBED2 {{'embed2' declared here}}
+ ::new(static_cast<void*>(&f1)) decltype(f1){1};
+ }
+#if __cplusplus >= 202002L
+ // expected-warning@+1 {{field 'embed2' requires explicit initialization but is not explicitly initialized}} expected-note@#FIELD_EMBED2 {{'embed2' declared here}}
+ explicit F(const char(&)[2]) : f1(1) {}
+#else
+ explicit F(const char(&)[2]) : f1{1, 2} { }
+#endif
+ // expected-warning@+1 {{field 'embed2' requires explicit initialization but is not explicitly initialized}} expected-note@#FIELD_EMBED2 {{'embed2' declared here}}
+ explicit F(const char(&)[3]) : f1{} {}
+ // expected-warning@+1 {{field 'embed2' requires explicit initialization but is not explicitly initialized}} expected-note@#FIELD_EMBED2 {{'embed2' declared here}}
+ explicit F(const char(&)[4]) : f1{1} {}
+ // expected-warning@+1 {{field 'embed2' requires explicit initialization but is not explicitly initialized}} expected-note@#FIELD_EMBED2 {{'embed2' declared here}}
+ explicit F(const char(&)[5]) : f1{.embed1 = 1} {}
+ };
+ F ctors[] = {
+ F(""),
+ F("_"),
+ F("__"),
+ F("___"),
+ F("____")
+ };
+ (void)ctors;
+
S::foo(S{1, 2, 3, 4});
S::foo(S{.s1 = 100, .s4 = 100});
S::foo(S{.s1 = 100}); // expected-warning {{field 's4' requires explicit initialization but is not explicitly initialized}} expected-note@#FIELD_S4 {{'s4' declared here}}
|
|
@AaronBallman Could we get this into the 20.x release before branching? (Sorry for the last-minute PR, I just noticed the bug.) |
|
Note the build error appears unrelated: |
|
@AaronBallman just a bump (hope the branch isn't cut yet) :) |
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
Sorry for the delayed review, this fell off my radar this week! :-/
Changes LGTM! The release branch did get cut, so this would need to be backported (https://releases.llvm.org/19.1.0/docs/GitHub.html#backporting-fixes-to-the-release-branches).
Looks like (according to item 6 here) https://en.cppreference.com/w/cpp/language/direct_initialization That member init is always 'direct' initialization. |
Ah okay no worries, thanks! So I guess you'll need to merge this into mainline and then I can make a PR to backport it? |
Yup! I was just waiting on Erich for a second set of eyes on the changes before I landed, but it's pushed now. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/50/builds/9651 Here is the relevant piece of the build log for the reference |
Oh, @AaronBallman - would you mind adding this as a milestone so I can |
Error: Command failed due to missing milestone. |
|
There we go, added the right milestone this time. :-) |
|
/cherry-pick 20fd7df |
|
/pull-request #125249 |
…lang::require_field_initialization]] (llvm#124329) It turns out we weren't handling one case: the value-initialization of a field inside a struct. I'm not sure why this falls under `IK_Direct` rather than `IK_Value` in Clang, but it seems to work. (cherry picked from commit 20fd7df)
|
It looks like this commit introduces an undesired false-positive: https://gcc.godbolt.org/z/rEjzK63he I don't think it's helpful to warn about not explicitly initializing a field on copy or move. Another example: https://gcc.godbolt.org/z/con9dTrPb Please fix or revert (unless this indeed intended, which I hope is not the case). |
|
Thanks! This was reported in #126490, we'll track it there. |
6 participants
It turns out we weren't handling one case: the value-initialization of a field inside a struct.
I'm not sure why this falls under
IK_Directrather thanIK_Valuein Clang, but it seems to work.