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.

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

clang-format: PointerAlignment: Right not working with tab indentation. #55407

Closed
ashleypittman opened this issue May 12, 2022 · 20 comments · Fixed by llvm/llvm-project-release-prs#15
Labels
bug Indicates an unexpected problem or unintended behavior clang-format release:backport release:merged release:reviewed

Comments

@ashleypittman
Copy link

ashleypittman commented May 12, 2022

When using PointerAlignment: Right with Tabs then variable names get aligned incorrectly. This needs a combination of UseTab and PointerAlignment to reproduce, and probably AlignConsecutiveDeclarations as well in order to request the alignment. It's broken for several values of UseTab, although Never works as expected.

We are also using IndentWidth: 8, without this then UseTab ForIndentation appears to work, at least for first-level indents although UseTab Always is still broken.

Sample input file:

struct something_big {
    int value;
};

int main() {
    struct something_big big;
    char *ptr;
    int i;
    return 0;
}

Configuration

#UseTab: ForIndentation
UseTab: Always
PointerAlignment: Right
AlignConsecutiveDeclarations: true
IndentWidth: 8

Observed output, note the incorrect spacing before *ptr.

struct something_big {
	int value;
};

int main() {
	struct something_big big;
	char		*ptr;
	int		     i;

	return 0;
}

When running with PointerAlignment: Middle we see the output is correct, however this does not match our existing coding style.

struct something_big {
  int value;
};

int main() {
  struct something_big big;
  char *	       ptr;
  int		       i;

  return 0;
}

Verified on clang-format version 14.0.0 (Fedora 14.0.0-1.fc36) (fedora 36) and others.

@llvmbot
Copy link
Collaborator

llvmbot commented May 12, 2022

@llvm/issue-subscribers-clang-format

@mkurdej mkurdej added the bug Indicates an unexpected problem or unintended behavior label May 13, 2022
@llvmbot
Copy link
Collaborator

llvmbot commented May 13, 2022

@llvm/issue-subscribers-bug

@mkurdej
Copy link
Member

mkurdej commented May 13, 2022

Confirmed on main.
There's another problem, when using UseTab: ForIndentation, we get a tab after char in the line:

	char	        *ptr;

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

mkurdej commented May 13, 2022

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

@ashleypittman
Copy link
Author

Thank you. I'm doing a half-day today but I'll aim to compile and test this Monday.

@ashleypittman
Copy link
Author

I can confirm that it works as expected with the patch applied. Thank you very much for a fast turnaround.

@github-actions github-actions bot removed the awaiting-review Has pending Phabricator review label May 16, 2022
@ashleypittman
Copy link
Author

Is the fix for this in 14.0.4? I've installed the release today and am still seeing multiple issues.

@tstellar
Copy link
Collaborator

tstellar commented Jun 3, 2022

@mkurdej Is this OK to backport?

@mkurdej
Copy link
Member

mkurdej commented Jun 3, 2022

Yes, absolutely!

@tru
Copy link
Collaborator

tru commented Jun 6, 2022

Should be fine to backport @tstellar

@tstellar
Copy link
Collaborator

tstellar commented Jun 8, 2022

/cherry-pick e20bc89

@llvmbot
Copy link
Collaborator

llvmbot commented Jun 8, 2022

/branch llvm/llvm-project-release-prs/issue55407

llvmbot pushed a commit to llvm/llvm-project-release-prs that referenced this issue Jun 8, 2022
…tation.

Fixes llvm/llvm-project#55407.

Given configuration:
```
UseTab: Always
PointerAlignment: Right
AlignConsecutiveDeclarations: true
```

Before, the pointer was misaligned in this code:
```
void f() {
	unsigned long long big;
	char	      *ptr; // misaligned
	int		   i;
}
```

That was due to the fact that when handling right-aligned pointers, the Spaces were changed but StartOfTokenColumn was not.

Also, a tab was used not only for indentation but for spacing too when using `UseTab: ForIndentation` config option:
```
void f() {
	unsigned long long big;
	char	      *ptr; // \t after char
	int                i;
}
```

Reviewed By: owenpan

Differential Revision: https://reviews.llvm.org/D125528

(cherry picked from commit e20bc89)
@llvmbot
Copy link
Collaborator

llvmbot commented Jun 8, 2022

/pull-request llvm/llvm-project-release-prs#15

@mkurdej
Copy link
Member

mkurdej commented Jun 8, 2022

/branch mkurdej/llvm-project/gh55407-14.x

@mkurdej
Copy link
Member

mkurdej commented Jun 8, 2022

@tstellar, this should fix the automatic merge.

@llvmbot
Copy link
Collaborator

llvmbot commented Jun 8, 2022

Failed to create pull request for gh55407-14.x https://github.com/llvm/llvm-project/actions/runs/2459565902

@mkurdej
Copy link
Member

mkurdej commented Jun 8, 2022

Not sure what failed here... Any hints?

@tru
Copy link
Collaborator

tru commented Jun 8, 2022

I think that happens when you use the branch flow - it's a limitation in that one right now - @tstellar fixed a similar issue manually before. If you describe it Tom - I can also help out fixing them until the proper fix is in place.

@tstellar
Copy link
Collaborator

tstellar commented Jun 8, 2022

@tru The fix is to copy the branch to the llvm/llvm-project-release-prs repo and then use the new branch for the pull request.

tstellar pushed a commit to llvm/llvm-project-release-prs that referenced this issue Jun 9, 2022
…tation.

Fixes llvm/llvm-project#55407.

Given configuration:
```
UseTab: Always
PointerAlignment: Right
AlignConsecutiveDeclarations: true
```

Before, the pointer was misaligned in this code:
```
void f() {
	unsigned long long big;
	char	      *ptr; // misaligned
	int		   i;
}
```

That was due to the fact that when handling right-aligned pointers, the Spaces were changed but StartOfTokenColumn was not.

Also, a tab was used not only for indentation but for spacing too when using `UseTab: ForIndentation` config option:
```
void f() {
	unsigned long long big;
	char	      *ptr; // \t after char
	int                i;
}
```

Reviewed By: owenpan

Differential Revision: https://reviews.llvm.org/D125528
@tstellar
Copy link
Collaborator

tstellar commented Jun 9, 2022

Merged: 3cd9df8

sunxfancy pushed a commit to sunxfancy/LLVM-IPRA that referenced this issue Jul 13, 2022
…tation.

Fixes llvm/llvm-project#55407.

Given configuration:
```
UseTab: Always
PointerAlignment: Right
AlignConsecutiveDeclarations: true
```

Before, the pointer was misaligned in this code:
```
void f() {
	unsigned long long big;
	char	      *ptr; // misaligned
	int		   i;
}
```

That was due to the fact that when handling right-aligned pointers, the Spaces were changed but StartOfTokenColumn was not.

Also, a tab was used not only for indentation but for spacing too when using `UseTab: ForIndentation` config option:
```
void f() {
	unsigned long long big;
	char	      *ptr; // \t after char
	int                i;
}
```

Reviewed By: owenpan

Differential Revision: https://reviews.llvm.org/D125528
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
…tation.

Fixes llvm/llvm-project#55407.

Given configuration:
```
UseTab: Always
PointerAlignment: Right
AlignConsecutiveDeclarations: true
```

Before, the pointer was misaligned in this code:
```
void f() {
	unsigned long long big;
	char	      *ptr; // misaligned
	int		   i;
}
```

That was due to the fact that when handling right-aligned pointers, the Spaces were changed but StartOfTokenColumn was not.

Also, a tab was used not only for indentation but for spacing too when using `UseTab: ForIndentation` config option:
```
void f() {
	unsigned long long big;
	char	      *ptr; // \t after char
	int                i;
}
```

Reviewed By: owenpan

Differential Revision: https://reviews.llvm.org/D125528
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior clang-format release:backport release:merged release:reviewed
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants