[clang-format] Add SpacesInParensOption for filtering repeated parens #77522
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-format @llvm/pr-subscribers-clang Author: Gedare Bloom (gedare) ChangesThe __attribute((specifier-list)) currently is formatted based on the SpacesInParensOptions.Other (previously, SpacesInParentheses). This change allows finer control over addition of spaces between the consecutive parens, and between the inner parens and the list of attribute specifiers. Differential Revision: https://reviews.llvm.org/D155529 This is migrated from Phabricator, see more discussion there. I will next provide an option for SpacesInParensOptions.Doubled to control (and override) injection of spaces within doubled parens Full diff: https://github.com/llvm/llvm-project/pull/77522.diff 7 Files Affected:
diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index 3d42571e82d8a0..bfd393eb033459 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -5625,6 +5625,13 @@ the configuration (without a prefix: ``Auto``).
InConditionalStatements: true
Other: true
+ * ``bool InAttributeSpecifiers`` Put a space in parentheses of attribute specifiers.
+
+ .. code-block:: c++
+
+ true: false:
+ __attribute__( ( noreturn ) ) vs. __attribute__((noreturn))
+
* ``bool InConditionalStatements`` Put a space in parentheses only inside conditional statements
(``for/if/while/switch...``).
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index ddeb1186d65ac8..4a23286c2b61a7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1079,6 +1079,8 @@ clang-format
- Add ``ObjCPropertyAttributeOrder`` which can be used to sort ObjC property
attributes (like ``nonatomic, strong, nullable``).
- Add ``.clang-format-ignore`` files.
+- Add ``InAttributeSpecifiers`` style option to ``SpacesInParensOptions``
+ to control addition of spaces after the ``__attribute__`` keyword.
libclang
--------
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 8604dea689f937..c9f0617711157d 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -4515,6 +4515,12 @@ struct FormatStyle {
/// Other: true
/// \endcode
struct SpacesInParensCustom {
+ /// Put a space in parentheses of attribute specifiers.
+ /// \code
+ /// true: false:
+ /// __attribute__( ( noreturn ) ) vs. __attribute__((noreturn))
+ /// \endcode
+ bool InAttributeSpecifiers;
/// Put a space in parentheses only inside conditional statements
/// (``for/if/while/switch...``).
/// \code
@@ -4548,17 +4554,20 @@ struct FormatStyle {
bool Other;
SpacesInParensCustom()
- : InConditionalStatements(false), InCStyleCasts(false),
- InEmptyParentheses(false), Other(false) {}
+ : InAttributeSpecifiers(false), InConditionalStatements(false),
+ InCStyleCasts(false), InEmptyParentheses(false), Other(false) {}
- SpacesInParensCustom(bool InConditionalStatements, bool InCStyleCasts,
+ SpacesInParensCustom(bool InAttributeSpecifiers,
+ bool InConditionalStatements, bool InCStyleCasts,
bool InEmptyParentheses, bool Other)
- : InConditionalStatements(InConditionalStatements),
+ : InAttributeSpecifiers(InAttributeSpecifiers),
+ InConditionalStatements(InConditionalStatements),
InCStyleCasts(InCStyleCasts), InEmptyParentheses(InEmptyParentheses),
Other(Other) {}
bool operator==(const SpacesInParensCustom &R) const {
- return InConditionalStatements == R.InConditionalStatements &&
+ return InAttributeSpecifiers == R.InAttributeSpecifiers &&
+ InConditionalStatements == R.InConditionalStatements &&
InCStyleCasts == R.InCStyleCasts &&
InEmptyParentheses == R.InEmptyParentheses && Other == R.Other;
}
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index f798d555bf9929..97a457f2733ade 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -753,6 +753,7 @@ template <> struct MappingTraits<FormatStyle::SpacesInLineComment> {
template <> struct MappingTraits<FormatStyle::SpacesInParensCustom> {
static void mapping(IO &IO, FormatStyle::SpacesInParensCustom &Spaces) {
+ IO.mapOptional("InAttributeSpecifiers", Spaces.InAttributeSpecifiers);
IO.mapOptional("InCStyleCasts", Spaces.InCStyleCasts);
IO.mapOptional("InConditionalStatements", Spaces.InConditionalStatements);
IO.mapOptional("InEmptyParentheses", Spaces.InEmptyParentheses);
@@ -1191,6 +1192,7 @@ template <> struct MappingTraits<FormatStyle> {
if (SpacesInParentheses) {
// set all options except InCStyleCasts and InEmptyParentheses
// to true for backward compatibility.
+ Style.SpacesInParensOptions.InAttributeSpecifiers = true;
Style.SpacesInParensOptions.InConditionalStatements = true;
Style.SpacesInParensOptions.InCStyleCasts =
SpacesInCStyleCastParentheses;
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index 8b43438c72dfe1..7dbc0d8f31819c 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -3999,10 +3999,18 @@ bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
}
if (Left.is(tok::l_paren) || Right.is(tok::r_paren)) {
- return (Right.is(TT_CastRParen) ||
- (Left.MatchingParen && Left.MatchingParen->is(TT_CastRParen)))
- ? Style.SpacesInParensOptions.InCStyleCasts
- : Style.SpacesInParensOptions.Other;
+ if (Right.is(TT_CastRParen) ||
+ (Left.MatchingParen && Left.MatchingParen->is(TT_CastRParen))) {
+ return Style.SpacesInParensOptions.InCStyleCasts;
+ }
+ const auto isAttributeParen = [](const FormatToken *Paren) {
+ return Paren && Paren->isOneOf(TT_AttributeLParen, TT_AttributeRParen);
+ };
+ if (isAttributeParen(&Left) || isAttributeParen(&Right) ||
+ isAttributeParen(Left.Previous) || isAttributeParen(Right.Next)) {
+ return Style.SpacesInParensOptions.InAttributeSpecifiers;
+ }
+ return Style.SpacesInParensOptions.Other;
}
if (Right.isOneOf(tok::semi, tok::comma))
return false;
diff --git a/clang/unittests/Format/ConfigParseTest.cpp b/clang/unittests/Format/ConfigParseTest.cpp
index 0c9f68f303d865..d537be9a11a910 100644
--- a/clang/unittests/Format/ConfigParseTest.cpp
+++ b/clang/unittests/Format/ConfigParseTest.cpp
@@ -231,6 +231,7 @@ TEST(ConfigParseTest, ParsesConfigurationBools) {
CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, AfterIfMacros);
CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, AfterOverloadedOperator);
CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, BeforeNonEmptyParentheses);
+ CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InAttributeSpecifiers);
CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InCStyleCasts);
CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InConditionalStatements);
CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InEmptyParentheses);
@@ -626,19 +627,23 @@ TEST(ConfigParseTest, ParsesConfiguration) {
Style.SpacesInParens = FormatStyle::SIPO_Never;
Style.SpacesInParensOptions = {};
CHECK_PARSE("SpacesInParentheses: true", SpacesInParensOptions,
- FormatStyle::SpacesInParensCustom(true, false, false, true));
+ FormatStyle::SpacesInParensCustom(true, true, false, false,
+ true));
Style.SpacesInParens = FormatStyle::SIPO_Never;
Style.SpacesInParensOptions = {};
CHECK_PARSE("SpacesInConditionalStatement: true", SpacesInParensOptions,
- FormatStyle::SpacesInParensCustom(true, false, false, false));
+ FormatStyle::SpacesInParensCustom(false, true, false, false,
+ false));
Style.SpacesInParens = FormatStyle::SIPO_Never;
Style.SpacesInParensOptions = {};
CHECK_PARSE("SpacesInCStyleCastParentheses: true", SpacesInParensOptions,
- FormatStyle::SpacesInParensCustom(false, true, false, false));
+ FormatStyle::SpacesInParensCustom(false, false, true, false,
+ false));
Style.SpacesInParens = FormatStyle::SIPO_Never;
Style.SpacesInParensOptions = {};
CHECK_PARSE("SpaceInEmptyParentheses: true", SpacesInParensOptions,
- FormatStyle::SpacesInParensCustom(false, false, true, false));
+ FormatStyle::SpacesInParensCustom(false, false, false, true,
+ false));
Style.SpacesInParens = FormatStyle::SIPO_Never;
Style.SpacesInParensOptions = {};
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 25ef5c680af862..97f397410f0002 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -16741,6 +16741,7 @@ TEST_F(FormatTest, ConfigurableSpacesInParens) {
Spaces.SpacesInParensOptions = {};
Spaces.SpacesInParensOptions.Other = true;
Spaces.SpacesInParensOptions.InConditionalStatements = true;
+ Spaces.SpacesInParensOptions.InAttributeSpecifiers = true;
verifyFormat("do_something( ::globalVar );", Spaces);
verifyFormat("call( x, y, z );", Spaces);
verifyFormat("call();", Spaces);
@@ -16815,6 +16816,12 @@ TEST_F(FormatTest, ConfigurableSpacesInParens) {
verifyFormat("void __attribute__((naked)) foo(int bar)", Spaces);
verifyFormat("void f( ) __attribute__((asdf));", Spaces);
+ Spaces.SpacesInParensOptions.InAttributeSpecifiers = true;
+ verifyFormat("SomeType *__attribute__( ( attr ) ) *a = NULL;", Spaces);
+ verifyFormat("void __attribute__( ( naked ) ) foo(int bar)", Spaces);
+ verifyFormat("void f( ) __attribute__( ( asdf ) );", Spaces);
+ Spaces.SpacesInParensOptions.InAttributeSpecifiers = false;
+
// Run the first set of tests again with:
Spaces.SpaceAfterCStyleCast = true;
verifyFormat("call(x, y, z);", Spaces);
|
|
I provide a sub-option to |
I expressed my opinion there:
So I still prefer that we have a boolean suboption (e.g. |
I will provide something shortly along these lines by duplicating the logic I used for this |
gedare
force-pushed
the
spacesinparens-attr-rebase-4
branch
from
060ad65
to
79b4c34
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I'm still making changes on this. Not sure how to mark that in GH. Need to add test cases for double parens, and get it working for the newly added suboptions. |
gedare
changed the title
Please have a look, I have moved this PR more toward this direction providing fine-grained controls over double parens. Someone will have to add the decltype understanding in order to make |
We should not go overboard with supporting all kinds of options/suboptions imaginable. I don't think we should support spaces between consecutive parens in the first place, which was likely a bug. We can't just disregard the long-standing policy on adding new options. If anyone thinks that policy is outdated, they should go through the RFC process and get it updated. |
Well, it seems to be far too late to go back and change the default behavior.
I have no problem with this policy. I think it is a good policy. I can advocate for my position. My patches relate to the style maintenance of a widely-used real-time operating system. The code is self-hosted and mirrored on GitHub with a documented style guide. In addition, I have been and will continue to be willing to provide bug fix and other maintenance support to clang-format based on the changes submitted to support these style needs. At the moment, I need to satisfy a particular style rule that requires me to add spaces inside of conditional and compound expressions. This support basically exists in If this is not desired, I will need to step back to consider my options further, and probably have to take this up with the other maintainers of my code base to see what, if any, concessions we can make based on what is allowed with |
It seems the RTEMS style guide you linked above doesn't have any examples for conditional expressions and compound expressions, and by conditional expressions I assume it means conditionals of control statements rather than ternary conditional expressions. Would |
Correct. Unfortunately, no one (myself) has taken the time to thoroughly document all examples. The existing style has been inherited from the original author of our code base, as these things tend to be for older projects.
Almost, it would also be We don't have We also have cases such as |
I'm ok with all of the above except for It seems adding a boolean sub-option that targets double pairs of parentheses as I suggested before is feasible although a better name than |
Yes, good point.
I'm fine to fix but request a concrete suggestion or proposal before I spend much more time on this. It might also work to suppress optionally the additional space on the first/last parenthesis pair, for these special "paired" parens. I think the way I already worked this patch is the right way to go to preserve backward compatibility while still allowing control over the repeated parentheses. Looks like it is called |
My idea was:
This way, we would avoid making all suboptions an |
OK, I think I understand. This would be greatly simplified if we can annotate the |
gedare
force-pushed
the
spacesinparens-attr-rebase-4
branch
from
e4b2d9b
to
d5a563b
Compare
This seems to work reasonably well for the cases I care about, as far as I can tell. It also works for a few additional cases I hadn't thought of that I picked up from the |
+1. |
|
I saw some comments on this. Looks like some were deleted maybe. This is still in my queue, but unclear when I'll get back to it. |
gedare
force-pushed
the
spacesinparens-attr-rebase-4
branch
from
d5a563b
to
a423067
Compare
gedare
changed the title
This change allows finer control over addition of spaces between a pair of consecutive opening parentheses with a pair of closing parentheses.
gedare
force-pushed
the
spacesinparens-attr-rebase-4
branch
from
0c1bc54
to
b76bafa
Compare
|
I had to rebase for merge conflicts, so I also cleaned up the commit history a bit. This seems to be working fine and tests pass. |
| /// Override any of the following options to prevent addition of space | ||
| /// between the first two parentheses in situations where a pair of | ||
| /// parentheses have been used. |
There was a problem hiding this comment.
This seems unclear or imprecise.
| /// decltype( ( x ) ) decltype((x)) | ||
| /// x = ( (int32)y ) x = ((int32))y | ||
| /// y = ( (int ( * )( int ))foo )( x ); y = ((int (*)(int))foo)(x); | ||
| /// __attribute__( ( noreturn ) ) __attribute__((noreturn)) |
There was a problem hiding this comment.
These new examples are a little confusing IMO. I would just delete them.
| @@ -1175,8 +1176,8 @@ template <> struct MappingTraits<FormatStyle> { | |||
| (SpacesInParentheses || SpaceInEmptyParentheses || | |||
| SpacesInConditionalStatement || SpacesInCStyleCastParentheses)) { | |||
| if (SpacesInParentheses) { | |||
| // set all options except InCStyleCasts and InEmptyParentheses | |||
| // to true for backward compatibility. | |||
| // for backward compatibility. | |||
There was a problem hiding this comment.
Capitalize the first letter of "for".
Yeah, sorry, I posted a comment here but decided it was a bit irrelevant. :) But going to ask anyway then. Can this setting remove the space from repeated parentheses pattern like this? foo( (void *)buf, size );to: foo((void *)buf, size );So it does remove it on one side where the C-style cast parentheses, but doesn't on the other side? |
The __attribute((specifier-list)) currently is formatted based on the SpacesInParensOptions.Other (previously, SpacesInParentheses). This change allows finer control over addition of spaces between the consecutive parens, and between the inner parens and the list of attribute specifiers.
Differential Revision: https://reviews.llvm.org/D155529
This is migrated from Phabricator, see more discussion there.