Skip to content

[clang-format] Don't confuse initializer equal signs in for loops #77712

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

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

rymiel
Copy link
Member

@rymiel rymiel commented Jan 11, 2024

clang-format has logic to align declarations of multiple variables of the same type, aligning them at the equals sign. This logic is applied in for loops as well. However, this alignment logic also erroneously affected the equals signs of designated initializers.

This patch forbids alignment if the token 2 tokens back from the equals sign is a designated initializer period.

Fixes #73902

clang-format has logic to align declarations of multiple variables of
the same type, aligning them at the equals sign. This logic is applied
in for loops as well. However, this alignment logic also erroneously
affected the equals signs of designated initializers.

This patch forbids alignment if the token 2 tokens back from the equals
sign is a designated initializer period.

Fixes llvm#73902
@llvmbot
Copy link
Member

llvmbot commented Jan 11, 2024

@llvm/pr-subscribers-clang-format

Author: Emilia Kond (rymiel)

Changes

clang-format has logic to align declarations of multiple variables of the same type, aligning them at the equals sign. This logic is applied in for loops as well. However, this alignment logic also erroneously affected the equals signs of designated initializers.

This patch forbids alignment if the token 2 tokens back from the equals sign is a designated initializer period.

Fixes #73902


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

2 Files Affected:

  • (modified) clang/lib/Format/ContinuationIndenter.cpp (+3-1)
  • (modified) clang/unittests/Format/FormatTest.cpp (+12)
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index 102504182c4505..ce6c17c3b7d927 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -703,7 +703,9 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
 
   if (Current.is(tok::equal) &&
       (State.Line->First->is(tok::kw_for) || Current.NestingLevel == 0) &&
-      CurrentState.VariablePos == 0) {
+      CurrentState.VariablePos == 0 &&
+      (!Previous.Previous ||
+       Previous.Previous->isNot(TT_DesignatedInitializerPeriod))) {
     CurrentState.VariablePos = State.Column;
     // Move over * and & if they are bound to the variable name.
     const FormatToken *Tok = &Previous;
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 25ef5c680af862..01678a4971caae 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -4944,6 +4944,18 @@ TEST_F(FormatTest, DesignatedInitializers) {
                "    [3] = cccccccccccccccccccccccccccccccccccccc,\n"
                "    [4] = dddddddddddddddddddddddddddddddddddddd,\n"
                "    [5] = eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee};");
+
+  verifyFormat("for (const TestCase &test_case : {\n"
+               "         TestCase{\n"
+               "             .a = 1,\n"
+               "             .b = 1,\n"
+               "         },\n"
+               "         TestCase{\n"
+               "             .a = 2,\n"
+               "             .b = 2,\n"
+               "         },\n"
+               "     }) {\n"
+               "}\n");
 }
 
 TEST_F(FormatTest, BracedInitializerIndentWidth) {

CurrentState.VariablePos == 0) {
CurrentState.VariablePos == 0 &&
(!Previous.Previous ||
Previous.Previous->isNot(TT_DesignatedInitializerPeriod))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need to search further back if the element being initialized is an array.
struct { int x[2]; } a[] = { [0].x = {1,1}, [1].x[0] = 42};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a for loop, but similar code should be possible in a for loop initialization

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we handle that in seperate issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, see if anyone complains anyway.

Copy link
Contributor

@mydeveloperday mydeveloperday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me I'm happy if we keep it to this use case only.

CurrentState.VariablePos == 0) {
CurrentState.VariablePos == 0 &&
(!Previous.Previous ||
Previous.Previous->isNot(TT_DesignatedInitializerPeriod))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we handle that in seperate issue?

@rymiel rymiel merged commit fa6025e into llvm:main Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clang-format misindents field initializers inside range-based for initializer
4 participants