[clang] Add -Wmissing-designated-field-initializers #81364

vvd170501 · 2024-02-10T16:17:51Z

#56628 changed the behavior of -Wmissing-field-initializers, which introduces many new warnings in C++ code that uses partial designated initializers. If such code is being built with -Wextra -Werror, this change will break the build.

This PR adds a new flag that allows to disable these new warnings and keep the old ones, as was suggested by @AaronBallman in the original issue: #56628 (comment)

github-actions · 2024-02-10T16:18:11Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-02-10T16:34:58Z

@llvm/pr-subscribers-clang

Author: Vadim D. (vvd170501)

Changes

Fixes #68933.

#56628 changed the behavior of -Wmissing-field-initializers, which introduces many new warnings in C++ code that uses partial designated initializers. If such code is being built with -Wextra -Werror, this change will break the build.

This PR adds a new flag that allows to disable these new warnings, as was suggested by @AaronBallman in the original issue: #56628 (comment)

Full diff: https://github.com/llvm/llvm-project/pull/81364.diff

4 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticGroups.td (+9-1)
(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+4)
(modified) clang/lib/Sema/SemaInit.cpp (+26-24)
(modified) clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp (+6-5)

diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 975eca0ad9b642..bda533f77cc56a 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -516,7 +516,15 @@ def MethodSignatures : DiagGroup<"method-signatures">;
 def MismatchedParameterTypes : DiagGroup<"mismatched-parameter-types">;
 def MismatchedReturnTypes : DiagGroup<"mismatched-return-types">;
 def MismatchedTags : DiagGroup<"mismatched-tags">;
-def MissingFieldInitializers : DiagGroup<"missing-field-initializers">;
+def MissingDesignatedFieldInitializers : DiagGroup<"missing-designated-field-initializers">{
+  code Documentation = [{
+Warn about designated initializers with some fields missing (only in C++).
+  }];
+}
+// Default -Wmissing-field-initializers matches gcc behavior,
+// but missing-designated-field-initializers can be turned off to match old clang behavior.
+def MissingFieldInitializers : DiagGroup<"missing-field-initializers",
+                                         [MissingDesignatedFieldInitializers]>;
 def ModuleLock : DiagGroup<"module-lock">;
 def ModuleBuild : DiagGroup<"module-build">;
 def ModuleImport : DiagGroup<"module-import">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b4dc4feee8e63a..69e197e26b9b45 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6165,6 +6165,10 @@ def ext_initializer_string_for_char_array_too_long : ExtWarn<
 def warn_missing_field_initializers : Warning<
   "missing field %0 initializer">,
   InGroup<MissingFieldInitializers>, DefaultIgnore;
+// The same warning, but another group is needed to disable it separately.
+def warn_missing_designated_field_initializers : Warning<
+  "missing field %0 initializer">,
+  InGroup<MissingDesignatedFieldInitializers>, DefaultIgnore;
 def warn_braces_around_init : Warning<
   "braces around %select{scalar |}0initializer">,
   InGroup<DiagGroup<"braced-scalar-init">>;
diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp
index b6de06464cd6f3..08aa50ebad331f 100644
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@@ -2227,8 +2227,6 @@ void InitListChecker::CheckStructUnionTypes(
   size_t NumRecordDecls = llvm::count_if(RD->decls(), [&](const Decl *D) {
     return isa<FieldDecl>(D) || isa<RecordDecl>(D);
   });
-  bool CheckForMissingFields =
-    !IList->isIdiomaticZeroInitializer(SemaRef.getLangOpts());
   bool HasDesignatedInit = false;
 
   llvm::SmallPtrSet<FieldDecl *, 4> InitializedFields;
@@ -2269,11 +2267,6 @@ void InitListChecker::CheckStructUnionTypes(
       }
 
       InitializedSomething = true;
-
-      // Disable check for missing fields when designators are used.
-      // This matches gcc behaviour.
-      if (!SemaRef.getLangOpts().CPlusPlus)
-        CheckForMissingFields = false;
       continue;
     }
 
@@ -2285,7 +2278,7 @@ void InitListChecker::CheckStructUnionTypes(
     // These are okay for randomized structures. [C99 6.7.8p19]
     //
     // Also, if there is only one element in the structure, we allow something
-    // like this, because it's really not randomized in the tranditional sense.
+    // like this, because it's really not randomized in the traditional sense.
     //
     //   struct foo h = {bar};
     auto IsZeroInitializer = [&](const Expr *I) {
@@ -2363,23 +2356,32 @@ void InitListChecker::CheckStructUnionTypes(
   }
 
   // Emit warnings for missing struct field initializers.
-  if (!VerifyOnly && InitializedSomething && CheckForMissingFields &&
-      !RD->isUnion()) {
-    // It is possible we have one or more unnamed bitfields remaining.
-    // Find first (if any) named field and emit warning.
-    for (RecordDecl::field_iterator it = HasDesignatedInit ? RD->field_begin()
-                                                           : Field,
-                                    end = RD->field_end();
-         it != end; ++it) {
-      if (HasDesignatedInit && InitializedFields.count(*it))
-        continue;
+  if (!VerifyOnly && InitializedSomething && !RD->isUnion()) {
+    // Disable missing fields check for:
+    // - Zero initializers
+    // - Designated initializers (only in C). This matches gcc behaviour.
+    bool DisableCheck =
+        IList->isIdiomaticZeroInitializer(SemaRef.getLangOpts()) ||
+        (HasDesignatedInit && !SemaRef.getLangOpts().CPlusPlus);
+
+    if (!DisableCheck) {
+      // It is possible we have one or more unnamed bitfields remaining.
+      // Find first (if any) named field and emit warning.
+      for (RecordDecl::field_iterator it = HasDesignatedInit ? RD->field_begin()
+                                                             : Field,
+                                      end = RD->field_end();
+           it != end; ++it) {
+        if (HasDesignatedInit && InitializedFields.count(*it))
+          continue;
 
-      if (!it->isUnnamedBitfield() && !it->hasInClassInitializer() &&
-          !it->getType()->isIncompleteArrayType()) {
-        SemaRef.Diag(IList->getSourceRange().getEnd(),
-                     diag::warn_missing_field_initializers)
-            << *it;
-        break;
+        if (!it->isUnnamedBitfield() && !it->hasInClassInitializer() &&
+            !it->getType()->isIncompleteArrayType()) {
+          auto Diag = HasDesignatedInit
+                          ? diag::warn_missing_designated_field_initializers
+                          : diag::warn_missing_field_initializers;
+          SemaRef.Diag(IList->getSourceRange().getEnd(), Diag) << *it;
+          break;
+        }
       }
     }
   }
diff --git a/clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp b/clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
index 510ace58c35a6a..1e9c5fa082d077 100644
--- a/clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
+++ b/clang/test/SemaCXX/cxx2a-initializer-aggregates.cpp
@@ -4,7 +4,8 @@
 // RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,reorder -Wno-c99-designator -Werror=reorder-init-list -Wno-initializer-overrides
 // RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,override -Wno-c99-designator -Wno-reorder-init-list -Werror=initializer-overrides
 // RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected -Wno-c99-designator -Wno-reorder-init-list -Wno-initializer-overrides
-// RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,wmissing -Wmissing-field-initializers -Wno-c99-designator -Wno-reorder-init-list -Wno-initializer-overrides
+// RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,wmissing,wmissing-designated -Wmissing-field-initializers -Wno-c99-designator -Wno-reorder-init-list -Wno-initializer-overrides
+// RUN: %clang_cc1 -std=c++20 %s -verify=cxx20,expected,wmissing -Wmissing-field-initializers -Wno-missing-designated-field-initializers -Wno-c99-designator -Wno-reorder-init-list -Wno-initializer-overrides
 
 
 namespace class_with_ctor {
@@ -50,11 +51,11 @@ A a3 = {
 A a4 = {
   .x = 1, // override-note {{previous}}
   .x = 1 // override-error {{overrides prior initialization}}
-}; // wmissing-warning {{missing field 'y' initializer}}
+}; // wmissing-designated-warning {{missing field 'y' initializer}}
 A a5 = {
   .y = 1, // override-note {{previous}}
   .y = 1 // override-error {{overrides prior initialization}}
-}; // wmissing-warning {{missing field 'x' initializer}}
+}; // wmissing-designated-warning {{missing field 'x' initializer}}
 B b2 = {.a = 1}; // pedantic-error {{brace elision for designated initializer is a C99 extension}}
                  // wmissing-warning@-1 {{missing field 'y' initializer}}
 B b3 = {.a = 1, 2}; // pedantic-error {{mixture of designated and non-designated}} pedantic-note {{first non-designated}} pedantic-error {{brace elision}}
@@ -74,8 +75,8 @@ C c = {
 struct Foo { int a, b; };
 
 struct Foo foo0 = { 1 }; // wmissing-warning {{missing field 'b' initializer}}
-struct Foo foo1 = { .a = 1 }; // wmissing-warning {{missing field 'b' initializer}}
-struct Foo foo2 = { .b = 1 }; // wmissing-warning {{missing field 'a' initializer}}
+struct Foo foo1 = { .a = 1 }; // wmissing-designated-warning {{missing field 'b' initializer}}
+struct Foo foo2 = { .b = 1 }; // wmissing-designated-warning {{missing field 'a' initializer}}
 
 }

AaronBallman

Thank you for this improvement! Please add a release note to clang/docs/ReleaseNotes.rst so users know about the new diagnostic group.

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/lib/Sema/SemaInit.cpp

clang/docs/ReleaseNotes.rst

…-f-i check only when needed

vvd170501 · 2024-03-04T13:32:59Z

Ping

AaronBallman

Changes mostly LGTM aside from some minor nits.

clang/docs/ReleaseNotes.rst

AaronBallman · 2024-03-04T18:57:27Z

clang/lib/Sema/SemaInit.cpp

+  // This matches gcc behaviour.
+  if (!VerifyOnly && InitializedSomething && !RD->isUnion() &&
+      !IList->isIdiomaticZeroInitializer(SemaRef.getLangOpts()) &&
+      !(HasDesignatedInit && !SemaRef.getLangOpts().CPlusPlus)) {


Suggested change

!(HasDesignatedInit && !SemaRef.getLangOpts().CPlusPlus)) {

(!HasDesignatedInit || SemaRef.getLangOpts().CPlusPlus)) {

Minor preference for extracting the condition out into well-named bool variables, this is getting to be a little bit of a mess.

@AaronBallman, unexpanded condition corresponds to the comment I left a few lines above:

This check is disabled for designated initializers in C.

For me personally, it looks a bit less readable in expanded form ("check unused fields if there are no designated initializers or if language is C++")

@erichkeane, I did something like this before 8386461, but this adds another indentation level.

It's possible to calculate DisableCheck before the if statement, but it'll probably be less optimal than the current solution - if VerifyOnly is true, DisableCheck is not needed.

I'm not asking to add a level/'if'. I'm asking for declaring a set of bools above this 'if' that better reflect what is being tested. With HasDesignatedInit/CPlusPlus checks only being vaguely related, and the 'sort' of the checks not really being clear, I am asking for the list of '!'s to be shorter/grouped into a couple of 'bools' with descriptive names.

I am asking for the list of '!'s to be shorter/grouped into a couple of 'bools' with descriptive names

Like this? Other checks seem unrelated to each other and I can't see any reason to group them.

+ bool IsCDesignatedInitializer = HasDesignatedInit && !SemaRef.getLangOpts().CPlusPlus; if (!VerifyOnly && InitializedSomething && !RD->isUnion() && !IList->isIdiomaticZeroInitializer(SemaRef.getLangOpts()) && - !(HasDesignatedInit && !SemaRef.getLangOpts().CPlusPlus)) { + !IsCDesignatedInitializer) {

That one definitely helps... I was hoping they grouped better. But I guess if that is what we can get. Basically my issue is how difficult it was to deduce what I was looking at with all hte conditions. Perhaps Aaron has a suggestion.

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

github-actions · 2024-03-04T22:43:52Z

✅ With the latest revision this PR passed the C/C++ code formatter.

github-actions · 2024-03-05T16:28:23Z

@vvd170501 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may recieve a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

vvd170501 force-pushed the separate-missing-designated-field-initializers branch from aa48713 to b7e3ffb Compare February 10, 2024 16:20

vvd170501 marked this pull request as ready for review February 10, 2024 16:34

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Feb 10, 2024

AaronBallman requested review from AaronBallman, Fznamznon and shafik February 12, 2024 14:33

AaronBallman reviewed Feb 12, 2024

View reviewed changes

clang/include/clang/Basic/DiagnosticSemaKinds.td Show resolved Hide resolved

clang/lib/Sema/SemaInit.cpp Outdated Show resolved Hide resolved

vvd170501 force-pushed the separate-missing-designated-field-initializers branch from b7e3ffb to 2a9028e Compare February 12, 2024 22:33

vvd170501 commented Feb 12, 2024

View reviewed changes

clang/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

vvd170501 requested a review from AaronBallman February 19, 2024 09:11

shafik mentioned this pull request Feb 27, 2024

c++ missing-field-initializer warning regression on trunk/18rc. #83070

Closed

vvd170501 added 4 commits March 4, 2024 12:55

Add -Wmissing-designated-field-initializers, decide whether to skip m…

f73060f

…-f-i check only when needed

Reuse summary

1dc1b14

Reduce indentation

8386461

Update release notes

61a4489

vvd170501 force-pushed the separate-missing-designated-field-initializers branch from 2a9028e to 61a4489 Compare March 4, 2024 09:59

AaronBallman approved these changes Mar 4, 2024

View reviewed changes

vvd170501 and others added 3 commits March 5, 2024 00:02

Fix flag description in release notes

d21b608

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

Fix flag description in release notes (2)

495bc05

Co-authored-by: Aaron Ballman <aaron@aaronballman.com>

Add bool variable

fa70ace

Fix formatting

56d8dc6

erichkeane approved these changes Mar 4, 2024

View reviewed changes

AaronBallman merged commit 7df43cc into llvm:main Mar 5, 2024

vvd170501 deleted the separate-missing-designated-field-initializers branch March 5, 2024 18:06

PiotrZSL mentioned this pull request Apr 26, 2024

[clang-tidy] readability-redundant-member-init vs -Wmissing-field-initializers #90235

Open

	!(HasDesignatedInit && !SemaRef.getLangOpts().CPlusPlus)) {
	(!HasDesignatedInit \|\| SemaRef.getLangOpts().CPlusPlus)) {

Uh oh!

[clang] Add -Wmissing-designated-field-initializers #81364

[clang] Add -Wmissing-designated-field-initializers #81364

Uh oh!

Conversation

vvd170501 commented Feb 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 10, 2024

Uh oh!

llvmbot commented Feb 10, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vvd170501 commented Mar 4, 2024

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AaronBallman Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

erichkeane Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

vvd170501 Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvd170501 Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

erichkeane Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

vvd170501 Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichkeane Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vvd170501 commented Feb 10, 2024 •

edited

Loading

vvd170501 Mar 4, 2024 •

edited

Loading

vvd170501 Mar 4, 2024 •

edited

Loading

github-actions bot commented Mar 4, 2024 •

edited

Loading