[clang] [clang-format] Don't apply severe penalty if no possible column formats (PR #76675)
https://github.com/owenca closed https://github.com/llvm/llvm-project/pull/76675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Don't apply severe penalty if no possible column formats (PR #76675)
https://github.com/owenca approved this pull request. https://github.com/llvm/llvm-project/pull/76675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Don't apply severe penalty if no possible column formats (PR #76675)
https://github.com/HazardyKnusperkeks approved this pull request. As far as I'm concerned it looks good. But I need a second opinion. @mydeveloperday @owenca @rymiel https://github.com/llvm/llvm-project/pull/76675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Don't apply severe penalty if no possible column formats (PR #76675)
HazardyKnusperkeks wrote: > > I think that's not the right way to fix the issue. > > Why are the 2 lines formatted differently? It seems to me that this fixes > > the symptom, not the cause. > > Because for the line where brackets are used it meets the condition in the > next `if` statement down: > > https://github.com/llvm/llvm-project/blob/7e405eb722e40c79b7726201d0f76b5dab34ba0f/clang/lib/Format/FormatToken.cpp#L122-L125 > > ... and a penalty of zero is returned. Similar exclusion exists for > precomputing the column formats: > > https://github.com/llvm/llvm-project/blob/7e405eb722e40c79b7726201d0f76b5dab34ba0f/clang/lib/Format/FormatToken.cpp#L189-L192 > > But this ultimately ends up simply as a performance optimization as they're > never considered later in any case. > > Whereas if the braces are used instead of brackets it does not meet the _not > a left brace_ condition, and gets to the severe penalty return: > > https://github.com/llvm/llvm-project/blob/7e405eb722e40c79b7726201d0f76b5dab34ba0f/clang/lib/Format/FormatToken.cpp#L136-L140 > > This is because suitable column formats are not found, as there was less than > 5 commas found in the list (there's other conditions dependent on style > configuration further up) when precomputing the column formats, and so none > are generated: > > https://github.com/llvm/llvm-project/blob/7e405eb722e40c79b7726201d0f76b5dab34ba0f/clang/lib/Format/FormatToken.cpp#L271-L274 > > Seems a little odd to return severe penalty if no column formats are > available as opposed to having them, but none fit right now. > > But I'm thinking maybe better to somehow add a condition that if the left > brace appears within a short lambda body that'll end up on a single line then > a penalty of zero is also returned. More/better targeted change. > > Ultimately I believe comes down to finding a good justification to return > zero penalty when the braces are used inside lambda bodies. Maybe affects > other _short_ functions too like inlines - need to check. Only now I see the difference between the two lines... https://github.com/llvm/llvm-project/pull/76675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Don't apply severe penalty if no possible column formats (PR #76675)
jamesg-nz wrote: > I think that's not the right way to fix the issue. > > Why are the 2 lines formatted differently? It seems to me that this fixes the > symptom, not the cause. Because for the line where brackets are used it meets the condition in the next `if` statement down: https://github.com/llvm/llvm-project/blob/7e405eb722e40c79b7726201d0f76b5dab34ba0f/clang/lib/Format/FormatToken.cpp#L122 ... and a penalty of zero is returned. Similar exclusion exists for precomputing the column formats: https://github.com/llvm/llvm-project/blob/7e405eb722e40c79b7726201d0f76b5dab34ba0f/clang/lib/Format/FormatToken.cpp#L191 But this ultimately ends up simply as performance optimization as they're never considered later in any case. Whereas if the braces are used instead of brackets it does not meet the _not a left brace_ condition, and gets to the severe penalty return: https://github.com/llvm/llvm-project/blob/7e405eb722e40c79b7726201d0f76b5dab34ba0f/clang/lib/Format/FormatToken.cpp#L140 . This is because suitable column formats are not found, as there was less than 5 commas found in the list when precomputing the column formats, and so none are generated: https://github.com/llvm/llvm-project/blob/7e405eb722e40c79b7726201d0f76b5dab34ba0f/clang/lib/Format/FormatToken.cpp#L273 Seems a little odd to return severe penalty if no column formats are available as opposed to having them, but none fit right now. But I'm thinking maybe better to somehow add a condition that if the left brace appears within a short lambda body that'll end up on a single line then a penalty of zero is also returned. More/better targeted change. Ultimately I believe comes down to finding a good justification to return zero penalty when the braces are used inside lambda bodies. Maybe affects other _short_ functions too like inlines - need to check. https://github.com/llvm/llvm-project/pull/76675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Don't apply severe penalty if no possible column formats (PR #76675)
HazardyKnusperkeks wrote: I think that's not the right way to fix the issue. Why are the 2 lines formatted differently? It seems to me that this fixes the symptom, not the cause. https://github.com/llvm/llvm-project/pull/76675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Don't apply severe penalty if no possible column formats (PR #76675)
jamesg-nz wrote: Running debug build clang-format with `-debug` parameter against code provided in #56350 (and its duplicate #58469) you see `Penalty for line: 1`. So pretty obvious the [severe penalty](https://github.com/llvm/llvm-project/blob/703e83611cd8bb7174ae76ba2e301f5a5e88b905/clang/lib/Format/FormatToken.cpp#L140) is responsible. Thoughts on this change? Is the removal of the severe penalty being applied too broadly? ... or is it correct/OK? https://github.com/llvm/llvm-project/pull/76675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Don't apply severe penalty if no possible column formats (PR #76675)
llvmbot wrote:
@llvm/pr-subscribers-clang-format
Author: James Grant (jamesg-nz)
Changes
If there are possible column formats, but they weren't selected because they
don't fit within remaining characters for the current path then applying severe
penalty to induce column layout by selection of a different path seems fair.
But if due to style configuration or what the input code is, there are no
possible column formats, different paths aren't going to have column layouts.
Seems wrong to apply the severe penalty to induce column layouts if there are
none available.
It just causes selection of sub-optimal paths, e.g. get bad formatting when
brace initializers are used inside lambda bodies.
Fixes #56350
---
Full diff: https://github.com/llvm/llvm-project/pull/76675.diff
2 Files Affected:
- (modified) clang/lib/Format/FormatToken.cpp (+2-2)
- (modified) clang/unittests/Format/FormatTest.cpp (+15)
``diff
diff --git a/clang/lib/Format/FormatToken.cpp b/clang/lib/Format/FormatToken.cpp
index 7a2df8c53952f9..b791c5a26bbe3a 100644
--- a/clang/lib/Format/FormatToken.cpp
+++ b/clang/lib/Format/FormatToken.cpp
@@ -113,8 +113,8 @@ unsigned CommaSeparatedList::formatAfterToken(LineState
,
if (!State.NextToken || !State.NextToken->Previous)
return 0;
- if (Formats.size() == 1)
-return 0; // Handled by formatFromToken
+ if (Formats.size() <= 1)
+return 0; // Handled by formatFromToken (1) or avoid severe penalty (0).
// Ensure that we start on the opening brace.
const FormatToken *LBrace =
diff --git a/clang/unittests/Format/FormatTest.cpp
b/clang/unittests/Format/FormatTest.cpp
index 881993ede17c3d..b7350b2fe66d9b 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -13875,6 +13875,21 @@ TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
getLLVMStyleWithColumns(35));
verifyFormat("aa(aaa, {},\n"
" aaa);");
+
+ // No possible column formats, don't want the optimal paths penalized.
+ verifyFormat(
+ "waarudo::unit desk = {\n"
+ ".s = \"desk\", .p = p, .b = [] { return w::r{3, 10} * w::m; }};");
+ verifyFormat("SomeType something1([](const Input ) -> Output { return "
+ "Output{1, 2}; },\n"
+ "[](const Input ) -> Output { return "
+ "Output{1, 2}; });");
+ FormatStyle NoBinPacking = getLLVMStyle();
+ NoBinPacking.BinPackParameters = false;
+ verifyFormat("waarudo::unit desk = {\n"
+ ".s = \"desk\", .p = p, .b = [] { return w::r{3, 10, 1, 1, "
+ "1, 1} * w::m; }};",
+ NoBinPacking);
}
TEST_F(FormatTest, PullTrivialFunctionDefinitionsIntoSingleLine) {
``
https://github.com/llvm/llvm-project/pull/76675
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Don't apply severe penalty if no possible column formats (PR #76675)
github-actions[bot] wrote: Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using `@` followed by their GitHub username. If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the [LLVM GitHub User Guide](https://llvm.org/docs/GitHub.html). You can also ask questions in a comment on this PR, on the [LLVM Discord](https://discord.com/invite/xS7Z362) or on the [forums](https://discourse.llvm.org/). https://github.com/llvm/llvm-project/pull/76675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-format] Don't apply severe penalty if no possible column formats (PR #76675)
https://github.com/jamesg-nz created
https://github.com/llvm/llvm-project/pull/76675
If there are possible column formats, but they weren't selected because they
don't fit within remaining characters for the current path then applying severe
penalty to induce column layout by selection of a different path seems fair.
But if due to style configuration or what the input code is, there are no
possible column formats, different paths aren't going to have column layouts.
Seems wrong to apply the severe penalty to induce column layouts if there are
none available.
It just causes selection of sub-optimal paths, e.g. get bad formatting when
brace initializers are used inside lambda bodies.
Fixes #56350
>From 68f7dee10e20b4ceeaba67f1ebe6b1153647e6ff Mon Sep 17 00:00:00 2001
From: James Grant <42079499+jamesg...@users.noreply.github.com>
Date: Mon, 1 Jan 2024 21:35:11 +1300
Subject: [PATCH] [clang-format] Don't apply severe penalty if no possible
column formats
If there are possible column formats, but they weren't selected because
they don't fit within remaining characters for the current path then
applying severe penalty to induce column layout by selection of a
different path seems fair.
But if due to style configuration or what the input code is, there are
no possible column formats, different paths aren't going to have column
layouts. Seems wrong to apply the severe penalty to induce column
layouts if there are none available.
It just causes selection of sub-optimal paths, e.g. get bad formatting
when brace initializers are used inside lambda bodies.
Fixes #56350
---
clang/lib/Format/FormatToken.cpp | 4 ++--
clang/unittests/Format/FormatTest.cpp | 15 +++
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Format/FormatToken.cpp b/clang/lib/Format/FormatToken.cpp
index 7a2df8c53952f9..b791c5a26bbe3a 100644
--- a/clang/lib/Format/FormatToken.cpp
+++ b/clang/lib/Format/FormatToken.cpp
@@ -113,8 +113,8 @@ unsigned CommaSeparatedList::formatAfterToken(LineState
,
if (!State.NextToken || !State.NextToken->Previous)
return 0;
- if (Formats.size() == 1)
-return 0; // Handled by formatFromToken
+ if (Formats.size() <= 1)
+return 0; // Handled by formatFromToken (1) or avoid severe penalty (0).
// Ensure that we start on the opening brace.
const FormatToken *LBrace =
diff --git a/clang/unittests/Format/FormatTest.cpp
b/clang/unittests/Format/FormatTest.cpp
index 881993ede17c3d..b7350b2fe66d9b 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -13875,6 +13875,21 @@ TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
getLLVMStyleWithColumns(35));
verifyFormat("aa(aaa, {},\n"
" aaa);");
+
+ // No possible column formats, don't want the optimal paths penalized.
+ verifyFormat(
+ "waarudo::unit desk = {\n"
+ ".s = \"desk\", .p = p, .b = [] { return w::r{3, 10} * w::m; }};");
+ verifyFormat("SomeType something1([](const Input ) -> Output { return "
+ "Output{1, 2}; },\n"
+ "[](const Input ) -> Output { return "
+ "Output{1, 2}; });");
+ FormatStyle NoBinPacking = getLLVMStyle();
+ NoBinPacking.BinPackParameters = false;
+ verifyFormat("waarudo::unit desk = {\n"
+ ".s = \"desk\", .p = p, .b = [] { return w::r{3, 10, 1, 1, "
+ "1, 1} * w::m; }};",
+ NoBinPacking);
}
TEST_F(FormatTest, PullTrivialFunctionDefinitionsIntoSingleLine) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
