Author: Yuta Nakamura Date: 2026-08-22T19:20:44+03:00 New Revision: 6e294bcca832f6e02c0cbc0575393778bd953ce3
URL: https://github.com/llvm/llvm-project/commit/6e294bcca832f6e02c0cbc0575393778bd953ce3 DIFF: https://github.com/llvm/llvm-project/commit/6e294bcca832f6e02c0cbc0575393778bd953ce3.diff LOG: [clang-tidy] Fix false positives in readability-trailing-comma for designated initializers (#215934) The problem is that we delete the necessary comma whenever we use implicit initializer lists. How we solve this is that whenever we see an implicit initializer list, we do not match ``InitListExpr`` nodes at all, so that we will not delete the necessary comma. Why we chose this path is detailed in Alternatives considered. This produces a fix that breaks valid code (#214087) and one that never converges (#214086). <details> <summary><b>Alternatives considered</b></summary> ### Why not repair the source ranges instead The synthesized nodes also carry misleading locations - their range is a snapshot of the designator that caused them to be created, so it need not cover their own children. The anonymous-struct node in #214087 reports `9:5-9:10` while holding an initializer on line 10, which is why it measures as single-line. Repairing that would be a change to Clang rather than to the check, and it is not clear there is anything to repair: the semantic form exists to record which initializer belongs to which subobject, and nothing in Clang relies on its ranges bracketing their children. Only a source-rewriting tool needs that guarantee, and such a tool should not be inspecting nodes that were never written. Skipping them is both smaller and better scoped. ### Why not match on brace locations `getLBraceLoc()`/`getRBraceLoc()` are not a reliable discriminator. On the synthesized anonymous-struct node in #214087 both are *valid*, pointing at the `.` and at `a`. A validity check would suppress only one of the two false positives in #214087 and none of #214086. This was in fact the previous implementation of `isExplicit()`, replaced in #195175 for the same reason. </details> ## AI disclosure What Claude did - Explored the codebase to locate the check and the relevant Sema/AST machinery - Instrumented the check with temporary debug output and measured the actual InitListExpr properties on the reproducers (brace locations, isExplicit, computed policies) - Wrote the final two-line patch and all the added test cases - Found the isExplicit() history (PR #195175) that the fix depends on What I did - Directed the approach and interrogated the reasoning at each step - Reviewed and verified the results locally Fixes: #214086 Fixes: #214087 Added: Modified: clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp clang-tools-extra/docs/ReleaseNotes.md clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-cxx20.cpp clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma.c Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp b/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp index 4dd881cf37993..cb1a33ba09233 100644 --- a/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp @@ -84,7 +84,8 @@ void TrailingCommaCheck::registerMatchers(MatchFinder *Finder) { .bind("enum"), this); - Finder->addMatcher(initListExpr(unless(isEmptyInitList()), unless(isMacro())) + Finder->addMatcher(initListExpr(unless(isEmptyInitList()), unless(isMacro()), + unless(isImplicit())) .bind("initlist"), this); } diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index f9565fd73b8b0..51dd99256ca69 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -198,6 +198,12 @@ infrastructure are described first, followed by tool-specific sections. exclusively for overload resolution. Added the {option}`IgnoredTypes` option to allow customizing the set of ignored types. +- Improved {doc}`readability-trailing-comma + <clang-tidy/checks/readability/trailing-comma>` check by fixing false + positives on designated initializers, where initializer lists synthesized + for intermediate subobjects caused the trailing comma of the enclosing + list to be incorrectly rewritten. + - Improved {doc}`readability-use-std-min-max <clang-tidy/checks/readability/use-std-min-max>` check by fixing spurious trailing semicolons and lost comments when the `if` body has no braces. diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-cxx20.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-cxx20.cpp index b2f6ed072563f..db5a3fbeb2a57 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-cxx20.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-cxx20.cpp @@ -87,3 +87,122 @@ void with_array() { .count = 3, }; } + +struct AnonUnion { + int x; + union { struct { int a; int b; }; }; +}; + +void anonymous_union_members() { + AnonUnion w1 = { + .x = 1, + .a = 2, + .b = 3, + }; + + AnonUnion w2 = { + .x = 1, + .a = 2, + .b = 3 + }; + // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: initializer list should have a trailing comma + // CHECK-FIXES: AnonUnion w2 = { + // CHECK-FIXES-NEXT: .x = 1, + // CHECK-FIXES-NEXT: .a = 2, + // CHECK-FIXES-NEXT: .b = 3, + // CHECK-FIXES-NEXT: }; +} + +struct Inner { int v; }; +struct Nested { Inner x; Inner y; }; + +void nested_designator() { + Nested n1 = { + .x = {.v = 1}, + .y.v = 2, + }; + + Nested n2 = { + .x = {.v = 1}, + .y.v = 2 + }; + // CHECK-MESSAGES: :[[@LINE-2]]:13: warning: initializer list should have a trailing comma + // CHECK-FIXES: Nested n2 = { + // CHECK-FIXES-NEXT: .x = {.v = 1}, + // CHECK-FIXES-NEXT: .y.v = 2, + // CHECK-FIXES-NEXT: }; + + Nested n3 = { + .x = {.v = 1}, + .y = {.v = 2,}, + }; + // CHECK-MESSAGES: :[[@LINE-2]]:17: warning: initializer list should not have a trailing comma + // CHECK-FIXES: Nested n3 = { + // CHECK-FIXES-NEXT: .x = {.v = 1}, + // CHECK-FIXES-NEXT: .y = {.v = 2}, + // CHECK-FIXES-NEXT: }; +} + +struct AnonStruct { + int x; + struct { int p; int q; }; +}; + +void anonymous_struct_members() { + AnonStruct as1 = { + .x = 1, + .p = 2, + .q = 3, + }; + + AnonStruct as2 = { .x = 1, .p = 2, .q = 3, }; + // CHECK-MESSAGES: :[[@LINE-1]]:44: warning: initializer list should not have a trailing comma + // CHECK-FIXES: AnonStruct as2 = { .x = 1, .p = 2, .q = 3 }; +} + +struct Deep { int c; }; +struct Mid { Deep b; }; +struct Top { Mid a; }; + +void multi_level_designator() { + Top t1 = { + .a.b.c = 1, + }; + + Top t2 = { + .a.b.c = 1 + }; + // CHECK-MESSAGES: :[[@LINE-2]]:15: warning: initializer list should have a trailing comma + // CHECK-FIXES: Top t2 = { + // CHECK-FIXES-NEXT: .a.b.c = 1, + // CHECK-FIXES-NEXT: }; +} + +struct TwoFields { int v; int w; }; +struct Holder { TwoFields y; }; + +void repeated_subobject_designator() { + Holder h1 = { + .y.v = 1, + .y.w = 2, + }; +} + +struct WithArrayField { int vals[3]; int n; }; + +void array_designator() { + WithArrayField wa1 = { + .vals[0] = 1, + .n = 1, + }; + + WithArrayField wa2 = { + .vals[0] = 1, + .n = 1 + }; + // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: initializer list should have a trailing comma + // CHECK-FIXES: WithArrayField wa2 = { + // CHECK-FIXES-NEXT: .vals[0] = 1, + // CHECK-FIXES-NEXT: .n = 1, + // CHECK-FIXES-NEXT: }; +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma.c b/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma.c index bdf9912e54155..9498fead9fcda 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma.c +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma.c @@ -115,3 +115,54 @@ struct Point singleDesig4 = { // CHECK-FIXES: struct Point singleDesig4 = { // CHECK-FIXES-NEXT: .x = 10, // CHECK-FIXES-NEXT: }; + +struct AnonUnion { + int x; + union { struct { int a; int b; }; }; +}; + +struct AnonUnion au1 = { + .x = 1, + .a = 2, + .b = 3, +}; + +struct AnonUnion au2 = { + .x = 1, + .a = 2, + .b = 3 +}; +// CHECK-MESSAGES: :[[@LINE-2]]:9: warning: initializer list should have a trailing comma +// CHECK-FIXES: struct AnonUnion au2 = { +// CHECK-FIXES-NEXT: .x = 1, +// CHECK-FIXES-NEXT: .a = 2, +// CHECK-FIXES-NEXT: .b = 3, +// CHECK-FIXES-NEXT: }; + +struct Inner { int v; }; +struct Outer { struct Inner x; struct Inner y; }; + +struct Outer nd1 = { + .x = {.v = 1}, + .y.v = 2, +}; + +struct Outer nd2 = { + .x = {.v = 1}, + .y.v = 2 +}; +// CHECK-MESSAGES: :[[@LINE-2]]:11: warning: initializer list should have a trailing comma +// CHECK-FIXES: struct Outer nd2 = { +// CHECK-FIXES-NEXT: .x = {.v = 1}, +// CHECK-FIXES-NEXT: .y.v = 2, +// CHECK-FIXES-NEXT: }; + +struct Outer nd3 = { + .x = {.v = 1}, + .y = {.v = 2,}, +}; +// CHECK-MESSAGES: :[[@LINE-2]]:15: warning: initializer list should not have a trailing comma +// CHECK-FIXES: struct Outer nd3 = { +// CHECK-FIXES-NEXT: .x = {.v = 1}, +// CHECK-FIXES-NEXT: .y = {.v = 2}, +// CHECK-FIXES-NEXT: }; _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
