-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[clang-format] allow short function body on a single line #151428
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
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-format Author: Lidong Yan (brandb97) ChangesFix issue #145161 Full diff: https://github.com/llvm/llvm-project/pull/151428.diff 4 Files Affected:
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 31582a40de866..e13df9df83e18 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -878,6 +878,12 @@ struct FormatStyle {
/// \version 3.5
ShortFunctionStyle AllowShortFunctionsOnASingleLine;
+ /// Dependent on the value, function body like ``{ return 0; }`` can be
+ /// put on a single line. Only when AllowShortFunctionsOnASingleLine = None
+ /// and AllowShortBlocksOnASingleLine != Never, the value of this option
+ /// is true.
+ bool AllowShortFunctionBodiesOnASingleLine;
+
/// Different styles for handling short if statements.
enum ShortIfStyle : int8_t {
/// Never put short ifs on the same line.
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 063780721423f..61c0d399ca796 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -3828,6 +3828,9 @@ reformat(const FormatStyle &Style, StringRef Code,
default:
break;
}
+ Expanded.AllowShortFunctionBodiesOnASingleLine =
+ Expanded.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_None &&
+ Expanded.AllowShortBlocksOnASingleLine != FormatStyle::SBS_Never;
if (Expanded.DisableFormat)
return {tooling::Replacements(), 0};
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 0adf7ee9ed543..44c767a0db9b8 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -257,6 +257,12 @@ class LineJoiner {
return tryMergeSimpleBlock(I, E, Limit);
}
+ if (TheLine->Last->is(TT_FunctionLBrace) &&
+ TheLine->First == TheLine->Last &&
+ Style.AllowShortFunctionBodiesOnASingleLine) {
+ return tryMergeSimpleBlock(I, E, Limit);
+ }
+
const auto *PreviousLine = I != AnnotatedLines.begin() ? I[-1] : nullptr;
// Handle empty record blocks where the brace has already been wrapped.
if (PreviousLine && TheLine->Last->is(tok::l_brace) &&
@@ -532,6 +538,20 @@ class LineJoiner {
}
return MergedLines;
}
+
+ // Previously, UnwrappedLineParser would move the left brace to a new line
+ // when AllowShortFunctionBodiesOnASingleLine is enabled. However, if the
+ // function body cannot fit on a single line, and Style.BraceWrapping.AfterFunction
+ // is false, we should merge the function name and the left brace back onto
+ // the same line
+ if (NextLine.First->is(TT_FunctionLBrace) &&
+ Style.AllowShortFunctionBodiesOnASingleLine &&
+ !Style.BraceWrapping.AfterFunction) {
+ unsigned MergedLines = 0;
+ MergedLines = tryMergeSimpleBlock(I + 1, E, Limit);
+ return MergedLines > 0 ? 0 : 1;
+ }
+
auto IsElseLine = [&TheLine]() -> bool {
const FormatToken *First = TheLine->First;
if (First->is(tok::kw_else))
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 91b8fdc8a3c38..7dd59c1cbd678 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -1921,6 +1921,10 @@ void UnwrappedLineParser::parseStructuralElement(
}
} else if (Style.BraceWrapping.AfterFunction) {
addUnwrappedLine();
+ } else if (Style.AllowShortFunctionBodiesOnASingleLine) {
+ // Wrap the left brace here; we'll try to merge it back
+ // later if needed.
+ addUnwrappedLine();
}
if (!Previous || Previous->isNot(TT_TypeDeclarationParen))
FormatTok->setFinalizedType(TT_FunctionLBrace);
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are missing unit tests. clang/unittests/Format/FromatTest.cpp
.
7a074db
to
25d45a4
Compare
ping |
Can we fix the bug instead of adding an option? |
Sorry, I promised to add enum PutShortFunctionBodiesOnASingleLine option, but I haven’t touched this code in quite a while (due to summer vacation). It seems the requirements have changed since then, so maybe it was a good thing I didn’t write it after all? ;-) |
I can just fix the bug by moving |
@owenca I actually think adding a new option would be better. The reason is that when users want to specify putting function bodies on a single line, without a new option, they would have to set |
I don't think the requirements have changed from my perspective. See #145161 (comment).
We can't keep adding all kinds of options people can come up with. See https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options. |
I see, I’ll rewrite this part of the code. |
25d45a4
to
04cd330
Compare
@owenca Could you help review my code? Thank you very much. |
e5a880b
to
7cdbee9
Compare
const bool NextShortBlock = NextLine.First == NextLine.Last && | ||
I + 3 != E && I[3]->First->is(tok::r_brace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I + 2 == E
this is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I am being stupid, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't quite understand why the LLDB formatter failed. Could you help me analyze the possible reasons for the error? Thank you very much!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the question, where do you get LLDB from?
7cdbee9
to
0c5bc1a
Compare
When set AllowShortBlocksOnASingleLine as Empty or Always and we can't put the whole function on a single line, clang-format doesn't tries to put the function body on a seperate line from function signature. Add tryMergeLines to put function bodies on a single line if possible. Signed-off-by: Lidong Yan <yldhome2d2@gmail.com>
0c5bc1a
to
8df95ae
Compare
Fix issue #145161