[clang-format]: Split alignment of declarations around assignment #69340
+223
−49
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-format Author: Gedare Bloom (gedare) ChangesFunction pointers are detected as a type of declaration using FunctionTypeLParen. They are aligned based on rules for AlignConsecutiveDeclarations. When a function pointer is on the right-hand side of an assignment, the alignment of the function pointer can result in excessive whitespace padding due to the ordering of alignment, as the alignment processes a line from left-to-right and first aligns the declarations before and after the assignment operator, and then aligns the assignment operator. Injection of whitespace by alignment of declarations after the equal sign followed by alignment of the equal sign results in the excessive whitespace. Fixes #68079. Full diff: https://github.com/llvm/llvm-project/pull/69340.diff 3 Files Affected:
diff --git a/clang/lib/Format/WhitespaceManager.cpp b/clang/lib/Format/WhitespaceManager.cpp
index dc81060671c1712..e6bc0963b40dc75 100644
--- a/clang/lib/Format/WhitespaceManager.cpp
+++ b/clang/lib/Format/WhitespaceManager.cpp
@@ -108,9 +108,10 @@ const tooling::Replacements &WhitespaceManager::generateReplacements() {
calculateLineBreakInformation();
alignConsecutiveMacros();
alignConsecutiveShortCaseStatements();
- alignConsecutiveDeclarations();
+ alignConsecutiveDeclarationsPreAssignment();
alignConsecutiveBitFields();
alignConsecutiveAssignments();
+ alignConsecutiveDeclarationsPostAssignment();
alignChainedConditionals();
alignTrailingComments();
alignEscapedNewlines();
@@ -973,13 +974,51 @@ void WhitespaceManager::alignConsecutiveShortCaseStatements() {
Changes);
}
-void WhitespaceManager::alignConsecutiveDeclarations() {
+void WhitespaceManager::alignConsecutiveDeclarationsPreAssignment() {
if (!Style.AlignConsecutiveDeclarations.Enabled)
return;
AlignTokens(
Style,
[](Change const &C) {
+ for (FormatToken *Prev = C.Tok->Previous; Prev; Prev = Prev->Previous)
+ if (Prev->is(tok::equal))
+ return false;
+ if (C.Tok->isOneOf(TT_FunctionDeclarationName, TT_FunctionTypeLParen))
+ return true;
+ if (C.Tok->isNot(TT_StartOfName))
+ return false;
+ if (C.Tok->Previous &&
+ C.Tok->Previous->is(TT_StatementAttributeLikeMacro))
+ return false;
+ // Check if there is a subsequent name that starts the same declaration.
+ for (FormatToken *Next = C.Tok->Next; Next; Next = Next->Next) {
+ if (Next->is(tok::comment))
+ continue;
+ if (Next->is(TT_PointerOrReference))
+ return false;
+ if (!Next->Tok.getIdentifierInfo())
+ break;
+ if (Next->isOneOf(TT_StartOfName, TT_FunctionDeclarationName,
+ tok::kw_operator)) {
+ return false;
+ }
+ }
+ return true;
+ },
+ Changes, /*StartAt=*/0, Style.AlignConsecutiveDeclarations);
+}
+
+void WhitespaceManager::alignConsecutiveDeclarationsPostAssignment() {
+ if (!Style.AlignConsecutiveDeclarations.Enabled)
+ return;
+
+ AlignTokens(
+ Style,
+ [](Change const &C) {
+ for (FormatToken *Next = C.Tok->Next; Next; Next = Next->Next)
+ if (Next->is(tok::equal))
+ return false;
if (C.Tok->isOneOf(TT_FunctionDeclarationName, TT_FunctionTypeLParen))
return true;
if (C.Tok->isNot(TT_StartOfName))
diff --git a/clang/lib/Format/WhitespaceManager.h b/clang/lib/Format/WhitespaceManager.h
index df7e9add1cd446f..19bd4a3a6f7791f 100644
--- a/clang/lib/Format/WhitespaceManager.h
+++ b/clang/lib/Format/WhitespaceManager.h
@@ -227,7 +227,8 @@ class WhitespaceManager {
void alignConsecutiveBitFields();
/// Align consecutive declarations over all \c Changes.
- void alignConsecutiveDeclarations();
+ void alignConsecutiveDeclarationsPreAssignment();
+ void alignConsecutiveDeclarationsPostAssignment();
/// Align consecutive declarations over all \c Changes.
void alignChainedConditionals();
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 963fb8f4d441618..1dc05ff9de5f0fa 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -18783,6 +18783,15 @@ TEST_F(FormatTest, AlignConsecutiveDeclarations) {
" \"bb\"};\n"
"int bbbbbbb = 0;",
Alignment);
+ // http://llvm.org/PR68079
+ verifyFormat("using Fn = int (A::*)();\n"
+ "using RFn = int (A::*)() &;\n"
+ "using RRFn = int (A::*)() &&;",
+ Alignment);
+ verifyFormat("using Fn = int (A::*)();\n"
+ "using RFn = int *(A::*)() &;\n"
+ "using RRFn = double (A::*)() &&;",
+ Alignment);
// PAS_Right
verifyFormat("void SomeFunction(int parameter = 0) {\n"
|
HazardyKnusperkeks
approved these changes
Oct 18, 2023
|
@gedare can you fix the merge conflict on this one? |
owenca
requested changes
Oct 27, 2023
|
force push was required to layer the revert commit correctly. |
owenca
reviewed
Nov 10, 2023
There was a problem hiding this comment.
See #66857 (comment). Can you make this a suboption, e.g. AlignFunctionPointers? We can add another (e.g. AlignConstructorsDestructors) for ctors and dtors in the future.
Yes, I will work on this. |
mydeveloperday
approved these changes
Nov 17, 2023
|
@owenca this waits for re-review |
Can you rebase the patch? |
done |
owenca
reviewed
Jan 6, 2024
…r function pointers"" This reverts commit 7bc1031.
Function pointers are detected as a type of declaration using FunctionTypeLParen. They are aligned based on rules for AlignConsecutiveDeclarations. When a function pointer is on the right-hand side of an assignment, the alignment of the function pointer can result in excessive whitespace padding due to the ordering of alignment, as the alignment processes a line from left-to-right and first aligns the declarations before and after the assignment operator, and then aligns the assignment operator. Injection of whitespace by alignment of declarations after the equal sign followed by alignment of the equal sign results in the excessive whitespace. Fixes llvm#68079.
Fixed the comments and added a release note. |
owenca
approved these changes
Jan 10, 2024
owenca
reviewed
Jan 10, 2024
Co-authored-by: Owen Pan <owenpiano@gmail.com>
justinfargnoli
pushed a commit
to justinfargnoli/llvm-project
that referenced
this pull request
Jan 28, 2024
…vm#69340) Function pointers are detected as a type of declaration using FunctionTypeLParen. They are aligned based on rules for AlignConsecutiveDeclarations. When a function pointer is on the right-hand side of an assignment, the alignment of the function pointer can result in excessive whitespace padding due to the ordering of alignment, as the alignment processes a line from left-to-right and first aligns the declarations before and after the assignment operator, and then aligns the assignment operator. Injection of whitespace by alignment of declarations after the equal sign followed by alignment of the equal sign results in the excessive whitespace. Fixes llvm#68079.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Function pointers are detected as a type of declaration using FunctionTypeLParen. They are aligned based on rules for AlignConsecutiveDeclarations. When a function pointer is on the right-hand side of an assignment, the alignment of the function pointer can result in excessive whitespace padding due to the ordering of alignment, as the alignment processes a line from left-to-right and first aligns the declarations before and after the assignment operator, and then aligns the assignment operator. Injection of whitespace by alignment of declarations after the equal sign followed by alignment of the equal sign results in the excessive whitespace.
Fixes #68079.