-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[clang-format] add an option to insert a space only for non-code block empty braces, not for empty parentheses #93634
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
Conversation
@llvm/pr-subscribers-clang-format @llvm/pr-subscribers-clang Author: Kohei Asano (khei4) ChangesWhatThis patch introduces an option to insert a space between empty braces, especially for empty initializer. MotivationWebKit has its own toImpl(listenerRef)->use( {} ); $ python Tools/Scripts/check-webkit-style
ERROR: Source/WebKit/UIProcess/Inspector/win/WebInspectorUIProxyWin.cpp:222: Extra space after ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/Inspector/win/WebInspectorUIProxyWin.cpp:222: Extra space before ) [whitespace/parens] [2]
ERROR: Source/WebKit/UIProcess/Inspector/win/WebInspectorUIProxyWin.cpp:222: Missing space inside { }. [whitespace/braces] [5] A space in empty braces is required, but one in (also for empty) parens isn't allowed. Current clang-format could insert a space in empty parenthesis and block simultaneously by turning on SpacesInParensOptions.InEmptyParentheses, but formatting only braces couldn't be achieved. So we want to insert a space only for braces. NoteI guess compatibility is important, but including braces in parens options might have been miss-leading. If more descriptive option name and desirable changes exist, I'm very happy to hear that :) Full diff: https://github.com/llvm/llvm-project/pull/93634.diff 6 Files Affected:
diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index 6d092219877f9..a1944eec8582b 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -6237,18 +6237,30 @@ the configuration (without a prefix: ``Auto``).
true: false:
x = ( int32 )y vs. x = (int32)y
- * ``bool InEmptyParentheses`` Put a space in parentheses only if the parentheses are empty i.e. '()'
+ * ``bool InEmptyParentheses`` Put a space in parentheses and braces only if they are empty i.e. '()' or '{}'
.. code-block:: c++
true: false:
void f( ) { vs. void f() {
int x[] = {foo( ), bar( )}; int x[] = {foo(), bar()};
+ T a = { }; T a = {};
if (true) { if (true) {
f( ); f();
} }
} }
+ * ``bool InEmptyBraces`` Put a space in *only* braces, not for parentheses, only if the braces are empty i.e. '{}'
+
+ .. code-block:: c++
+
+ true: false:
+ void f() { vs. void f() {
+ T x = {}; T x = { };
+ g(x, {}); g(x, { });
+ } }
+
+
* ``bool Other`` Put a space in parentheses not covered by preceding options.
.. code-block:: c++
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 274b45d1bc586..4482f7fb49788 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -4673,6 +4673,17 @@ struct FormatStyle {
/// } }
/// \endcode
bool InEmptyParentheses;
+ /// Put a space in brackets only if the parentheses are empty i.e. '()'
+ /// \code
+ /// true: false:
+ /// void f( ) { vs. void f() {
+ /// int x[] = {foo( ), bar( )}; int x[] = {foo(), bar()};
+ /// if (true) { if (true) {
+ /// f( ); f();
+ /// } }
+ /// } }
+ /// \endcode
+ bool InEmptyBraces;
/// Put a space in parentheses not covered by preceding options.
/// \code
/// true: false:
@@ -4682,18 +4693,20 @@ struct FormatStyle {
SpacesInParensCustom()
: InConditionalStatements(false), InCStyleCasts(false),
- InEmptyParentheses(false), Other(false) {}
+ InEmptyParentheses(false), InEmptyBraces(false), Other(false) {}
SpacesInParensCustom(bool InConditionalStatements, bool InCStyleCasts,
- bool InEmptyParentheses, bool Other)
+ bool InEmptyParentheses, bool InEmptyBraces,
+ bool Other)
: InConditionalStatements(InConditionalStatements),
InCStyleCasts(InCStyleCasts), InEmptyParentheses(InEmptyParentheses),
- Other(Other) {}
+ InEmptyBraces(InEmptyBraces), Other(Other) {}
bool operator==(const SpacesInParensCustom &R) const {
return InConditionalStatements == R.InConditionalStatements &&
InCStyleCasts == R.InCStyleCasts &&
- InEmptyParentheses == R.InEmptyParentheses && Other == R.Other;
+ InEmptyParentheses == R.InEmptyParentheses &&
+ InEmptyBraces == R.InEmptyBraces && Other == R.Other;
}
bool operator!=(const SpacesInParensCustom &R) const {
return !(*this == R);
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 9cba0c2614eef..efcc48b910718 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -723,6 +723,7 @@ template <> struct MappingTraits<FormatStyle::SpacesInParensCustom> {
IO.mapOptional("InCStyleCasts", Spaces.InCStyleCasts);
IO.mapOptional("InConditionalStatements", Spaces.InConditionalStatements);
IO.mapOptional("InEmptyParentheses", Spaces.InEmptyParentheses);
+ IO.mapOptional("InEmptyBraces", Spaces.InEmptyBraces);
IO.mapOptional("Other", Spaces.Other);
}
};
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index 7c4c76a91f2c5..094c182c8d448 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -4341,6 +4341,11 @@ bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
Right.MatchingParen == &Left && Line.Children.empty()) {
return Style.SpaceInEmptyBlock;
}
+ if (Style.SpacesInParensOptions.InEmptyBraces &&
+ (Left.is(tok::l_brace) && Left.isNot(BK_Block) &&
+ Right.is(tok::r_brace) && Right.isNot(BK_Block))) {
+ return Style.SpacesInParensOptions.InEmptyBraces;
+ }
if ((Left.is(tok::l_paren) && Right.is(tok::r_paren)) ||
(Left.is(tok::l_brace) && Left.isNot(BK_Block) &&
Right.is(tok::r_brace) && Right.isNot(BK_Block))) {
diff --git a/clang/unittests/Format/ConfigParseTest.cpp b/clang/unittests/Format/ConfigParseTest.cpp
index 82e72f08ffb5e..c8918d4cec128 100644
--- a/clang/unittests/Format/ConfigParseTest.cpp
+++ b/clang/unittests/Format/ConfigParseTest.cpp
@@ -238,6 +238,7 @@ TEST(ConfigParseTest, ParsesConfigurationBools) {
CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InCStyleCasts);
CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InConditionalStatements);
CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InEmptyParentheses);
+ CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InEmptyBraces);
CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, Other);
}
@@ -619,20 +620,24 @@ TEST(ConfigParseTest, ParsesConfiguration) {
FormatStyle::SIPO_Custom);
Style.SpacesInParens = FormatStyle::SIPO_Never;
Style.SpacesInParensOptions = {};
- CHECK_PARSE("SpacesInParentheses: true", SpacesInParensOptions,
- FormatStyle::SpacesInParensCustom(true, false, false, true));
+ CHECK_PARSE(
+ "SpacesInParentheses: true", SpacesInParensOptions,
+ FormatStyle::SpacesInParensCustom(true, false, false, false, true));
Style.SpacesInParens = FormatStyle::SIPO_Never;
Style.SpacesInParensOptions = {};
- CHECK_PARSE("SpacesInConditionalStatement: true", SpacesInParensOptions,
- FormatStyle::SpacesInParensCustom(true, false, false, false));
+ CHECK_PARSE(
+ "SpacesInConditionalStatement: true", SpacesInParensOptions,
+ FormatStyle::SpacesInParensCustom(true, false, false, false, false));
Style.SpacesInParens = FormatStyle::SIPO_Never;
Style.SpacesInParensOptions = {};
- CHECK_PARSE("SpacesInCStyleCastParentheses: true", SpacesInParensOptions,
- FormatStyle::SpacesInParensCustom(false, true, false, false));
+ CHECK_PARSE(
+ "SpacesInCStyleCastParentheses: true", SpacesInParensOptions,
+ FormatStyle::SpacesInParensCustom(false, true, false, false, false));
Style.SpacesInParens = FormatStyle::SIPO_Never;
Style.SpacesInParensOptions = {};
- CHECK_PARSE("SpaceInEmptyParentheses: true", SpacesInParensOptions,
- FormatStyle::SpacesInParensCustom(false, false, true, false));
+ CHECK_PARSE(
+ "SpaceInEmptyParentheses: true", SpacesInParensOptions,
+ FormatStyle::SpacesInParensCustom(false, false, true, false, false));
Style.SpacesInParens = FormatStyle::SIPO_Never;
Style.SpacesInParensOptions = {};
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 2f0c0f0266774..f154941426f2c 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -14027,6 +14027,11 @@ TEST_F(FormatTest, LayoutCxx11BraceInitializers) {
SpaceBetweenBraces.SpacesInParens = FormatStyle::SIPO_Custom;
SpaceBetweenBraces.SpacesInParensOptions.InEmptyParentheses = true;
verifyFormat("vector< int > x{ };", SpaceBetweenBraces);
+ SpaceBetweenBraces.SpacesInParens = FormatStyle::SIPO_Custom;
+ SpaceBetweenBraces.SpacesInParensOptions.InEmptyParentheses = false;
+ SpaceBetweenBraces.SpacesInParensOptions.Other = false;
+ SpaceBetweenBraces.SpacesInParensOptions.InEmptyBraces = true;
+ verifyFormat("T x = { };\nf(x, { });\ng();", SpaceBetweenBraces);
}
TEST_F(FormatTest, FormatsBracedListsInColumnLayout) {
|
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 think you might be missing a change in the == operator for the Style
✅ With the latest revision this PR passed the C/C++ code formatter. |
d75153b
to
0b41025
Compare
See #93635 (comment). Also, we need to deprecate |
1a953e3
to
4cdd7bd
Compare
@owenca |
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.
In general this looks ok, but lets see what @owenca and @HazardyKnusperkeks say
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.
can these be Never/Always/Leave
clang/lib/Format/Format.cpp
Outdated
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.
can we confirm this matches their published style guide because we are about to change defaults
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 believe this matches, but it might be miss-leading to specify only Block and Initlist, (although they are same with all braces). check-webkit-style script seems like to check whatever empty braces have a space inside of it.
self.assert_lint(' { }', '')
self.assert_lint(' {}', 'Missing space inside { }. [whitespace/braces] [5]')
self.assert_lint(' { }', 'Too many spaces inside { }. [whitespace/braces] [5]')
So adding SIEBO_Always and using it might be better. To double check, I will ask in WebKit slack also. 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.
IMO we need to go by the published WebKit coding style, not the script.
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.
@owenca
Thank you. I found there are no entries about empty braces. I will consult on the WebKit mailing list about the inconsistency between Code Style Guideline and check-webkit-style script. I think it’s good to separate the WebKit Style fixes and empty braces options addition on this patch. Although I can’t still find how much this kind of inconsistency exist, I will file an issue if it’s necessary. :)
@khei4 maybe an With We have the following brace types now:
I would implement all that make sense or are applicable to WebKit. |
4f25089
to
02b4de5
Compare
@owenca Thank you for the review!
I make those variants. How do you think the behavior of config, which has only
TBH, I'm not sure how much braces should be classified on config, I will consult concretely, but |
8018daf
to
c4932b2
Compare
I confirmed the update of WebKit Code Style Guideline for empty braces. I make WebKit style insert a space in any empty braces. I also updated document by script. |
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 cannot remove options or you'll break everyone using that option right?
/// ``SpaceInEmptyBracesOptions`` to ``true``. | ||
/// \version 10 | ||
bool SpaceInEmptyBlock; | ||
// bool SpaceInEmptyBlock; |
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.
sorry I don't understand are you removing this?
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 was reviewing this PR and running into some issues related to https://reviews.llvm.org/D68415. I felt that we had to bite the bullet and add a new boolean option like the author did initially.
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.
Actually, we might be able to support the following enumerated values:
- Always (
WebKit
default) - Block (which would deprecate
SpaceInEmptyBlock
) - Leave (to be implemented in this patch or a follow-up patch)
- Never (
LLVM
default)
c4932b2
to
079f6d2
Compare
rebased but still required to be tested and fix again |
See #93634 (comment). |
See #153765. |
@khei4 can you close this pull request? |
@owenca Thank you! I'm sorry for not responding. |
Fixes #93635
Leave
option, which doesn't modify any empty braces.Note
I guess compatibility is important, but including braces in parens options might have been miss-leading. If more descriptive option name and desirable changes exist, I'm very happy to hear that :)