[PATCH] D25171: clang-format: Add two new formatting options
shafik added a comment. Herald added a project: All. Is this still relevant or should this be closed? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D25171/new/ https://reviews.llvm.org/D25171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25171: clang-format: Add two new formatting options
rsmmr added a comment. Well, last time I counted it was more like 100 contributors---Bro existed before GitHub (and before git). The style-guide is lacking, yes ... what can I say. I don't really want to argue about importance of the project. We would like to use clang-format here and elsewhere, and I'd think these options could be useful to others as well, so I created a patch and submitted. If you guys don't like it, your call. https://reviews.llvm.org/D25171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25171: clang-format: Add two new formatting options
djasper added a comment.
It's not about whether or not we like the patch. It's whether adding these
options is a good trade-off for clang-format overall. If we find that actually
more people would find these styles desirable, we can reconsider.
I have left some comments anyway in case you want to keep the patch around.
> Format.h:603
>
> + /// \brief If ``true``, spaces will be inserted around if/for/while
> conditions.
> + bool SpacesAroundConditions;
It's actually more than if/for/while.
> Format.cpp:355
> IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
> +IO.mapOptional("SpacesAfterNot",
> + Style.SpacesAfterNot);
Unnecessary linebreaks.
> TokenAnnotator.cpp:1989
> +if (Left.is(tok::l_paren) && Left.Previous &&
> + Left.Previous->isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for,
> tok::kw_while,
> + tok::kw_switch, TT_ForEachMacro))
Indent is off.
> TokenAnnotator.cpp:1989-1990
> +if (Left.is(tok::l_paren) && Left.Previous &&
> + Left.Previous->isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for,
> tok::kw_while,
> + tok::kw_switch, TT_ForEachMacro))
> +return true;
This should go into a helper function or lambda so that it can be reused. What
about catch? Also some of these aren't tested, e.g. the preprocessor ones.
> TokenAnnotator.cpp:1993
> +if (Right.is(tok::r_paren) && Right.MatchingParen &&
> Right.MatchingParen->Previous &&
> + Right.MatchingParen->Previous->isOneOf(tok::kw_if, tok::pp_elif,
> tok::kw_for, tok::kw_while,
> + tok::kw_switch,
> TT_ForEachMacro))
Indent is off.
> TokenAnnotator.cpp:2232
>}
> + if (Style.SpacesAfterNot && Left.is(tok::exclaim))
> +return true;
Note that at least JavaScript/TypeScript has a ! postfix operator, i.e. unary
operator that applies to the token before it. You definitely don't want to
always add a space after that.
> TokenAnnotator.cpp:2233
> + if (Style.SpacesAfterNot && Left.is(tok::exclaim))
> +return true;
>if (Left.is(TT_UnaryOperator))
Indent is off.
> TokenAnnotator.cpp:2252
> return false;
> - if (Right.is(tok::coloncolon) && !Left.isOneOf(tok::l_brace, tok::comment))
> + if (Right.is(tok::coloncolon) && !Left.isOneOf(tok::l_brace, tok::comment,
> tok::l_paren))
> return (Left.is(TT_TemplateOpener) &&
Is this related in any way? I don't see a test with ::
> FormatTest.cpp:11508
> + Spaces.SpacesAfterNot = true;
> + verifyFormat("if (! a)\n return;", Spaces);
> + verifyFormat("while (! (x || y))\n return;", Spaces);
While you have added some test here, I think the more debatable ones are
actually where there is also a space before the !, e.g.:
if (a && ! b) ..
return ! c;
bool x = ! y && z;
The last one is especially tricky, because it makes it less clear that ! binds
stronger than &&. Might seem obvious in this case, but IIRC, we fought quite a
few bugs of the form:
if (! a < b) ..
Until a warning was added in Clang.
https://reviews.llvm.org/D25171
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25171: clang-format: Add two new formatting options
djasper added a comment. Sorry, but that's actually not enough, at least at first sight. With 37 contributors total, bro is still quite small and only 12 of them have more than a handful of commits. And it doesn't have a real style guide. It has: https://www.bro.org/development/contribute.html#coding-guidelines, which basically says "adapt to the existing code structure" and "We don’t have many strict rules". https://reviews.llvm.org/D25171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25171: clang-format: Add two new formatting options
rsmmr added a comment. Sure, I'm aiming to use clang-format on a couple of open-source code bases using this style, with the main one being the Bro network security monitor, see www.bro.org and github.com/bro/bro (note the stars and forks :-) Bro is also featured on GitHub's list of security show cases, https://github.com/showcases/security. https://reviews.llvm.org/D25171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25171: clang-format: Add two new formatting options
rsmmr added a comment. Cool, thanks Steve. What's the next step for me to get it committed? https://reviews.llvm.org/D25171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25171: clang-format: Add two new formatting options
djasper added a comment. Could you read: http://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options And provide some evidence about the requirements for new style options? https://reviews.llvm.org/D25171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25171: clang-format: Add two new formatting options
elsteveogrande accepted this revision. elsteveogrande added a reviewer: elsteveogrande. elsteveogrande added a comment. This revision is now accepted and ready to land. Thought I'd take a look at the list of diffs and take a stab at a few (a few easy-looking and short ones :) ) to unblock some ppl, and unless @djasper has strong opinions about it I'd say why not. Looking at other rules "nearby" I see spaces inside and outside square-brackets and angle-brackets, so it makes sense to allow this for conditions, especially if someone thinks this helps make it more readable, and for `not` as well (I personally like that one, it's easy to eyeball right past that `!`. Also it appears to be off by default and I assume the unit tests work :) https://reviews.llvm.org/D25171 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25171: clang-format: Add two new formatting options
rsmmr created this revision.
rsmmr added reviewers: djasper, lodato.
rsmmr added a subscriber: cfe-commits.
Herald added a subscriber: klimek.
This patch adds two new options, feedback welcome.
SpacesAroundConditions (bool)
If true, spaces will be inserted around if/for/while conditions.
SpacesAfterNot (bool)
If true, spaces will be inserted after '!'.
https://reviews.llvm.org/D25171
Files:
docs/ClangFormatStyleOptions.rst
include/clang/Format/Format.h
lib/Format/Format.cpp
lib/Format/TokenAnnotator.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10194,6 +10194,8 @@
CHECK_PARSE_BOOL(SpaceAfterCStyleCast);
CHECK_PARSE_BOOL(SpaceAfterTemplateKeyword);
CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
+ CHECK_PARSE_BOOL(SpacesAfterNot);
+ CHECK_PARSE_BOOL(SpacesAroundConditions);
CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterClass);
CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterControlStatement);
@@ -11500,6 +11502,24 @@
verifyFormat("include \"a.td\"\ninclude \"b.td\"", Style);
}
+TEST_F(FormatTest, SpacesAfterNot) {
+ FormatStyle Spaces = getLLVMStyle();
+ Spaces.SpacesAfterNot = true;
+ verifyFormat("if (! a)\n return;", Spaces);
+ verifyFormat("while (! (x || y))\n return;", Spaces);
+ verifyFormat("do {\n} while (! foo);", Spaces);
+}
+
+TEST_F(FormatTest, SpacesAroundConditions) {
+ FormatStyle Spaces = getLLVMStyle();
+ Spaces.SpacesAroundConditions = true;
+ verifyFormat("if ( !a )\n return;", Spaces);
+ verifyFormat("if ( a )\n return;", Spaces);
+ verifyFormat("while ( a )\n return;", Spaces);
+ verifyFormat("while ( (a && b) )\n return;", Spaces);
+ verifyFormat("do {\n} while ( 1 != 0 );", Spaces);
+}
+
// Since this test case uses UNIX-style file path. We disable it for MS
// compiler.
#if !defined(_MSC_VER) && !defined(__MINGW32__)
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1984,6 +1984,16 @@
return Right.is(tok::hash);
if (Left.is(tok::l_paren) && Right.is(tok::r_paren))
return Style.SpaceInEmptyParentheses;
+ if (Style.SpacesAroundConditions) {
+if (Left.is(tok::l_paren) && Left.Previous &&
+ Left.Previous->isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
+ tok::kw_switch, TT_ForEachMacro))
+return true;
+if (Right.is(tok::r_paren) && Right.MatchingParen && Right.MatchingParen->Previous &&
+ Right.MatchingParen->Previous->isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
+ tok::kw_switch, TT_ForEachMacro))
+return true;
+ }
if (Left.is(tok::l_paren) || Right.is(tok::r_paren))
return (Right.is(TT_CastRParen) ||
(Left.MatchingParen && Left.MatchingParen->is(TT_CastRParen)))
@@ -2219,6 +2229,8 @@
return Style.SpacesInContainerLiterals;
return true;
}
+ if (Style.SpacesAfterNot && Left.is(tok::exclaim))
+return true;
if (Left.is(TT_UnaryOperator))
return Right.is(TT_BinaryOperator);
@@ -2237,7 +2249,7 @@
if (!Style.SpaceBeforeAssignmentOperators &&
Right.getPrecedence() == prec::Assignment)
return false;
- if (Right.is(tok::coloncolon) && !Left.isOneOf(tok::l_brace, tok::comment))
+ if (Right.is(tok::coloncolon) && !Left.isOneOf(tok::l_brace, tok::comment, tok::l_paren))
return (Left.is(TT_TemplateOpener) &&
Style.Standard == FormatStyle::LS_Cpp03) ||
!(Left.isOneOf(tok::identifier, tok::l_paren, tok::r_paren,
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -352,6 +352,10 @@
Style.SpacesInCStyleCastParentheses);
IO.mapOptional("SpacesInParentheses", Style.SpacesInParentheses);
IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
+IO.mapOptional("SpacesAfterNot",
+ Style.SpacesAfterNot);
+IO.mapOptional("SpacesAroundConditions",
+ Style.SpacesAroundConditions);
IO.mapOptional("Standard", Style.Standard);
IO.mapOptional("TabWidth", Style.TabWidth);
IO.mapOptional("UseTab", Style.UseTab);
@@ -557,6 +561,8 @@
LLVMStyle.SpaceBeforeParens = FormatStyle::SBPO_ControlStatements;
LLVMStyle.SpaceBeforeAssignmentOperators = true;
LLVMStyle.SpacesInAngles = false;
+ LLVMStyle.SpacesAfterNot = false;
+ LLVMStyle.SpacesAroundConditions = false;
LLVMStyle.PenaltyBreakComment = 300;
LLVMStyle.PenaltyBreakFirstLessLess = 120;
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++
