[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

In D108370#3020793 , @Sockke wrote:

> In D108370#3017800 , @aaron.ballman 
> wrote:
>
>> LGTM!
>
> Thanks for your review! I don't have commit access, here is my information:
> Name: liuke
> Email: liuke.ge...@bytedance.com

Thanks! I've commit on your behalf in e4902480f1e2f12f73c2b504e3d717536653dd7b 
.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

In D108370#3017800 , @aaron.ballman 
wrote:

> LGTM!

Thanks for your review! I don't have commit access, here is my information:
Name: liuke
Email: liuke.ge...@bytedance.com


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

In D108370#3017800 , @aaron.ballman 
wrote:

> LGTM!

Thanks for your review!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:484
+ AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (!FieldsToInit.count(F))
   return;

Sockke wrote:
> aaron.ballman wrote:
> > Given that we're touching this code, we might as well make clang-format 
> > happy with it (though I can't quite spot what it wants changed, so if it 
> > turns out to be a bad idea for some reason, I don't insist).
> > Given that we're touching this code, we might as well make clang-format 
> > happy with it (though I can't quite spot what it wants changed, so if it 
> > turns out to be a bad idea for some reason, I don't insist).
> 
> Yes, I have tried to format this code with clang-format but it does not work 
> perfectly.
Then as-is is fine by me, thank you!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:484
+ AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (!FieldsToInit.count(F))
   return;

aaron.ballman wrote:
> Given that we're touching this code, we might as well make clang-format happy 
> with it (though I can't quite spot what it wants changed, so if it turns out 
> to be a bad idea for some reason, I don't insist).
> Given that we're touching this code, we might as well make clang-format happy 
> with it (though I can't quite spot what it wants changed, so if it turns out 
> to be a bad idea for some reason, I don't insist).

Yes, I have tried to format this code with clang-format but it does not work 
perfectly.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke updated this revision to Diff 374430.
Sockke added a comment.

Update!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
@@ -516,3 +516,39 @@
 
 PositiveDefaultConstructorOutOfDecl::PositiveDefaultConstructorOutOfDecl() = 
default;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize 
these fields: F
+
+union U1 {
+  U1() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: union constructor should 
initialize one of these fields: X, K, Z, Y
+  int X;
+  // CHECK-FIXES: int X{};
+  union {
+int K;
+// CHECK-FIXES-NOT: int K{};
+  };
+  union {
+int Z;
+// CHECK-FIXES-NOT: int Z{};
+int Y;
+// CHECK-FIXES-NOT: int Y{};
+  };
+};
+
+union U2 {
+  U2() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: union constructor should 
initialize one of these fields: B, C, A
+  struct {
+int B;
+// CHECK-FIXES: int B{};
+union {
+  struct {
+PositiveMultipleConstructors Value;
+// CHECK-FIXES-NOT: PositiveMultipleConstructors Value{};
+  };
+  int C;
+  // CHECK-FIXES: int C{};
+};
+  };
+  int A;
+  // CHECK-FIXES-NOT: int A{};
+};
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -44,6 +44,23 @@
   }
 }
 
+template 
+void forEachFieldWithFilter(const RecordDecl , const T ,
+bool , Func &) {
+  for (const FieldDecl *F : Fields) {
+if (F->isAnonymousStructOrUnion()) {
+  if (const CXXRecordDecl *R = F->getType()->getAsCXXRecordDecl()) {
+AnyMemberHasInitPerUnion = false;
+forEachFieldWithFilter(*R, R->fields(), AnyMemberHasInitPerUnion, Fn);
+  }
+} else {
+  Fn(F);
+}
+if (Record.isUnion() && AnyMemberHasInitPerUnion)
+  break;
+  }
+}
+
 void removeFieldsInitializedInBody(
 const Stmt , ASTContext ,
 SmallPtrSetImpl ) {
@@ -461,8 +478,9 @@
   // Collect all fields but only suggest a fix for the first member of unions,
   // as initializing more than one union member is an error.
   SmallPtrSet FieldsToFix;
-  SmallPtrSet UnionsSeen;
-  forEachField(ClassDecl, OrderedFields, [&](const FieldDecl *F) {
+  bool AnyMemberHasInitPerUnion = false;
+  forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
+ AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (!FieldsToInit.count(F))
   return;
 // Don't suggest fixes for enums because we don't know a good default.
@@ -471,8 +489,8 @@
 if (F->getType()->isEnumeralType() ||
 (!getLangOpts().CPlusPlus20 && F->isBitField()))
   return;
-if (!F->getParent()->isUnion() || UnionsSeen.insert(F->getParent()).second)
-  FieldsToFix.insert(F);
+FieldsToFix.insert(F);
+AnyMemberHasInitPerUnion = true;
   });
   if (FieldsToFix.empty())
 return;


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
@@ -516,3 +516,39 @@
 
 PositiveDefaultConstructorOutOfDecl::PositiveDefaultConstructorOutOfDecl() = default;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these fields: F
+
+union U1 {
+  U1() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: union constructor should initialize one of these fields: X, K, Z, Y
+  int X;
+  // CHECK-FIXES: int X{};
+  union {
+int K;
+// CHECK-FIXES-NOT: int K{};
+  };
+  union {
+int Z;
+// CHECK-FIXES-NOT: int Z{};
+int Y;
+// CHECK-FIXES-NOT: int Y{};
+  };
+};
+
+union U2 {
+  U2() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: union constructor should initialize one of these fields: B, C, A
+  struct {
+int B;
+// CHECK-FIXES: int B{};
+union {
+  struct {
+PositiveMultipleConstructors Value;
+// CHECK-FIXES-NOT: PositiveMultipleConstructors Value{};
+  };
+  int C;
+  // CHECK-FIXES: int C{};
+};
+  };
+  int A;
+  // CHECK-FIXES-NOT: int A{};
+};
Index: 

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:484
+ AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (!FieldsToInit.count(F))
   return;

Given that we're touching this code, we might as well make clang-format happy 
with it (though I can't quite spot what it wants changed, so if it turns out to 
be a bad idea for some reason, I don't insist).



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp:526
+  union {
+int K;
+  };

aaron.ballman wrote:
> Can you add a `CHECK-FIXES-NOT` that we're not adding the fix for this case?
I think you missed this request? Same for the one immediately below.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp:534
+
+union U2 {
+  U2() {}

Sockke wrote:
> aaron.ballman wrote:
> > A related interesting test would be:
> > ```
> > union U3 {
> >   U3() {}
> > 
> >   struct {
> > int B;
> >   } b;
> >   int A;
> > };
> > ```
> > I would expect `A` or `b` to need initialization for the union, and `B` to 
> > need initialization for the struct.
> > A related interesting test would be:
> > ```
> > union U3 {
> >   U3() {}
> > 
> >   struct {
> > int B;
> >   } b;
> >   int A;
> > };
> > ```
> > I would expect `A` or `b` to need initialization for the union, and `B` to 
> > need initialization for the struct.
> 
> I apologize for the delay in responding. It seems to me that `struct {}b` has 
> no user-defined constructor, so all members are initialized to their 
> defaults, it is unnecessary to initialize `B` explicitly. 
Okay, fair point!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Hi, Could you please take some time to review this diff again? @aaron.ballman


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp:534
+
+union U2 {
+  U2() {}

aaron.ballman wrote:
> A related interesting test would be:
> ```
> union U3 {
>   U3() {}
> 
>   struct {
> int B;
>   } b;
>   int A;
> };
> ```
> I would expect `A` or `b` to need initialization for the union, and `B` to 
> need initialization for the struct.
> A related interesting test would be:
> ```
> union U3 {
>   U3() {}
> 
>   struct {
> int B;
>   } b;
>   int A;
> };
> ```
> I would expect `A` or `b` to need initialization for the union, and `B` to 
> need initialization for the struct.

I apologize for the delay in responding. It seems to me that `struct {}b` has 
no user-defined constructor, so all members are initialized to their defaults, 
it is unnecessary to initialize `B` explicitly. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp:526
+  union {
+int K;
+  };

Can you add a `CHECK-FIXES-NOT` that we're not adding the fix for this case?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp:529
+  union {
+int Z;
+int Y;

Same here.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp:534
+
+union U2 {
+  U2() {}

A related interesting test would be:
```
union U3 {
  U3() {}

  struct {
int B;
  } b;
  int A;
};
```
I would expect `A` or `b` to need initialization for the union, and `B` to need 
initialization for the struct.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Hi, Could anyone please review this diff?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

What do you think? @aaron.ballman @bkramer


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Any thoughts?  : )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Any thoughts? : )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke added a comment.

Any thoughts? : )




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:49
+void forEachFieldWithFilter(const RecordDecl , const T ,
+bool , Func &) {
+  for (const FieldDecl *F : Fields) {

MTC wrote:
> Can it be modified to the following form? Or further abstract `filter` into a 
> parameter to make this function more general.
> 
> 
> ```
> template 
> void forEachFieldWithFilter(const RecordDecl , const T ,
> bool , Func &) {
>   forEachField(Record, Fields, Fn);
>   if (Record.isUnion() && AnyMemberHasInitPerUnion)
> break;
> }
> ```
> Can it be modified to the following form? Or further abstract `filter` into a 
> parameter to make this function more general.
> 
> 
> ```
> template 
> void forEachFieldWithFilter(const RecordDecl , const T ,
> bool , Func &) {
>   forEachField(Record, Fields, Fn);
>   if (Record.isUnion() && AnyMemberHasInitPerUnion)
> break;
> }
> ```

This does not seem to work, because the "AnyMemberHasInitPerUnion" needs to be 
passed along the call stack.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[liushuai wang via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:49
+void forEachFieldWithFilter(const RecordDecl , const T ,
+bool , Func &) {
+  for (const FieldDecl *F : Fields) {

Can it be modified to the following form? Or further abstract `filter` into a 
parameter to make this function more general.


```
template 
void forEachFieldWithFilter(const RecordDecl , const T ,
bool , Func &) {
  forEachField(Record, Fields, Fn);
  if (Record.isUnion() && AnyMemberHasInitPerUnion)
break;
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108370/new/

https://reviews.llvm.org/D108370

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D108370: [clang-tidy] Fix wrong FixIt about union in cppcoreguidelines-pro-type-member-init

[gehry via Phabricator via cfe-commits]

Sockke created this revision.
Sockke added reviewers: aaron.ballman, bkramer, alexfh, malcolm.parsons, MTC, 
steven.zhang.
Herald added subscribers: shchenz, kbarton, xazax.hun, nemanjai.
Sockke requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

I can only initialize one member variable of the same level in the union. The 
case of anonymous records with multiple levels of nesting like the following 
also needs to meet this rule. The original logic is to horizontally obtain all 
the member variables in a record that need to be initialized and then filter to 
the variables that need to be fixed. Obviously, it is impossible to correctly 
initialize the desired variables according to the nesting relationship.

See Example 3 in class.union 

  union U {
U() {}
int x;  // int x{};
union {
  int k;  // int k{};  <==  wrong fix
};
union {
  int z;  // int z{};  <== wrong fix
  int y;
};
  };


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108370

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
@@ -516,3 +516,34 @@
 
 PositiveDefaultConstructorOutOfDecl::PositiveDefaultConstructorOutOfDecl() = 
default;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize 
these fields: F
+
+union U1 {
+  U1() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: union constructor should 
initialize one of these fields: X, K, Z, Y
+  int X;
+  // CHECK-FIXES: int X{};
+  union {
+int K;
+  };
+  union {
+int Z;
+int Y;
+  };
+};
+
+union U2 {
+  U2() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: union constructor should 
initialize one of these fields: B, C, A
+  struct {
+int B;
+// CHECK-FIXES: int B{};
+union {
+  struct {
+PositiveMultipleConstructors Value;
+  };
+  int C;
+  // CHECK-FIXES: int C{};
+};
+  };
+  int A;
+};
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
@@ -44,6 +44,23 @@
   }
 }
 
+template 
+void forEachFieldWithFilter(const RecordDecl , const T ,
+bool , Func &) {
+  for (const FieldDecl *F : Fields) {
+if (F->isAnonymousStructOrUnion()) {
+  if (const CXXRecordDecl *R = F->getType()->getAsCXXRecordDecl()) {
+AnyMemberHasInitPerUnion = false;
+forEachFieldWithFilter(*R, R->fields(), AnyMemberHasInitPerUnion, Fn);
+  }
+} else {
+  Fn(F);
+}
+if (Record.isUnion() && AnyMemberHasInitPerUnion)
+  break;
+  }
+}
+
 void removeFieldsInitializedInBody(
 const Stmt , ASTContext ,
 SmallPtrSetImpl ) {
@@ -461,8 +478,9 @@
   // Collect all fields but only suggest a fix for the first member of unions,
   // as initializing more than one union member is an error.
   SmallPtrSet FieldsToFix;
-  SmallPtrSet UnionsSeen;
-  forEachField(ClassDecl, OrderedFields, [&](const FieldDecl *F) {
+  bool AnyMemberHasInitPerUnion = false;
+  forEachFieldWithFilter(ClassDecl, ClassDecl.fields(),
+ AnyMemberHasInitPerUnion, [&](const FieldDecl *F) {
 if (!FieldsToInit.count(F))
   return;
 // Don't suggest fixes for enums because we don't know a good default.
@@ -471,8 +489,8 @@
 if (F->getType()->isEnumeralType() ||
 (!getLangOpts().CPlusPlus20 && F->isBitField()))
   return;
-if (!F->getParent()->isUnion() || UnionsSeen.insert(F->getParent()).second)
-  FieldsToFix.insert(F);
+FieldsToFix.insert(F);
+AnyMemberHasInitPerUnion = true;
   });
   if (FieldsToFix.empty())
 return;


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-member-init.cpp
@@ -516,3 +516,34 @@
 
 PositiveDefaultConstructorOutOfDecl::PositiveDefaultConstructorOutOfDecl() = default;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: constructor does not initialize these fields: F
+
+union U1 {
+  U1() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: union constructor should initialize one of these