Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[clang-format] Bad interaction between AlignConsecutiveAssignments.Enabled=true and BinPackArguments=false #55360

Closed
cha5on opened this issue May 9, 2022 · 2 comments
Closed

[clang-format] Bad interaction between AlignConsecutiveAssignments.Enabled=true and BinPackArguments=false #55360

cha5on opened this issue May 9, 2022 · 2 comments
Labels
clang-format

Comments

@cha5on
Copy link
Contributor

cha5on commented May 9, 2022 •
edited by VoltrexKeyva

Description

AlignConsecutiveAssignments doesn't align properly if BinPackArguments = false.

First noticed with clang-format 14.0.0, but present in current main branch.

Proposed fix is at https://reviews.llvm.org/D125162 .

Example 1

.clang-format for this example:

---
Language:        Cpp
# BasedOnStyle:  LLVM
AccessModifierOffset: -2
AlignConsecutiveAssignments: Consecutive
BinPackArguments: false
BinPackParameters: false

Expected:

struct A {
  int a;
  int b;
  int c;
};
struct B {
  A a1;
  A a2;
  A a3;
};
int a_long_name                  = 1;
int an_even_longer_name_for_wrap = 1;
auto b                           = B{{a_long_name, a_long_name, a_long_name},

                                     {an_even_longer_name_for_wrapping,
                                      an_even_longer_name_for_wrapping,
                                      an_even_longer_name_for_wrapping}};

Actual:

struct A {
  int a;
  int b;
  int c;
};
struct B {
  A a1;
  A a2;
  A a3;
};
int a_long_name                  = 1;
int an_even_longer_name_for_wrap = 1;
auto b                           = B{{a_long_name, a_long_name, a_long_name},

           {an_even_longer_name_for_wrapping,
                                      an_even_longer_name_for_wrapping,
                                      an_even_longer_name_for_wrapping}};

Example 2

A slightly smaller case with a reduced column limit.

.clang-format for this example:

---
Language:        Cpp
# BasedOnStyle:  LLVM
AccessModifierOffset: -2
AlignConsecutiveAssignments: Consecutive
BinPackArguments: false
BinPackParameters: false
ColumnLimit:     50

Expected:

struct A {
  int a;
  int b;
};
struct B {
  A a1;
  A a2;
};
int a_longer_name_for_wrap = 1;
// comment to prevent alignment
int a_long_name = 1;
auto b          = B({a_long_name, a_long_name},
                    {a_longer_name_for_wrap,
                     a_longer_name_for_wrap});

Actual:

struct A {
  int a;
  int b;
};
struct B {
  A a1;
  A a2;
};
int a_longer_name_for_wrap = 1;
// comment to prevent alignment
int a_long_name = 1;
auto b          = B({a_long_name, a_long_name},
           {a_longer_name_for_wrap,
                     a_longer_name_for_wrap});
@llvmbot
Copy link
Collaborator

llvmbot commented May 9, 2022

@llvm/issue-subscribers-clang-format

@mkurdej mkurdej added the awaiting-review Has pending Phabricator review label May 10, 2022
@mkurdej
Copy link
Member

mkurdej commented May 10, 2022

Revision URI: https://reviews.llvm.org/D125162.

@github-actions github-actions bot removed the awaiting-review Has pending Phabricator review label May 16, 2022
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
@cha5on
[clang-format] fix alignment w/o binpacked args
384435c 
The combination of

- AlignConsecutiveAssignments.Enabled = true
- BinPackArguments = false

would result in the first continuation line of a braced-init-list being
improperly indented (missing a shift) when in a continued function call.
Indentation was also wrong for braced-init-lists continuing a
direct-list-initialization.  Check for opening braced lists in
continuation and ensure that the correct shift occurs.

Fixes llvm/llvm-project#55360

Reviewed By: curdeius

Differential Revision: https://reviews.llvm.org/D125162
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
clang-format
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mkurdej @cha5on @EugeneZelenko @llvmbot