[PATCH] D14052: clang-format: Format std::function template parameter consistently inside function
jeanphilippeD created this revision.
jeanphilippeD added a reviewer: djasper.
jeanphilippeD added a subscriber: cfe-commits.
Herald added a subscriber: klimek.
The function type declared in an std::function template parameter is confused
for a cast:
Currently:
Pass: "std::function< void(int, int) > fct;", Spaces);
Fail: "void inFunction() { std::function< void(int, int) > fct; }",
Actual result "void inFunction() { std::function< void(int, int)> fct; }" (no
space between ")>")
Root cause:
-Inside a function definition, Line.MustBeDeclaration is not true.
-This allows the context IsExpression to be true.
"Contexts.back().IsExpression = !ParametersOfFunctionType && !IsForOrCatch;"
-which then allow the right parenthesis to be marked incorrectly as cast, and
the left there after.
"if (ParensAreType && !ParensCouldEndDecl && !IsSizeOfOrAlignOf &&
(Contexts.size() > 1 && Contexts[Contexts.size() - 2].IsExpression))
IsCast = true;"
Because at the time of this marking, the angle bracket is not yet established
as a template opener and closer, this fix address the issue by resetting the
type to unknown for the parenthesis.(Unknown is the type the parenthesis hold
in the case the line must be a declaration).
It seems there should be a better alternative, but I am not sure where I should
look.
The tests are updated in 2 places as this incorrect deduction result in
incorrect spacing before the angle bracket, but also inside the parenthesis if
space is required there but not in cast.
Run all the test in FormatTests project and spot checked clang format output
for TokenAnnotator.cpp and FormatTest.cpp is the same before and after the
patch.
http://reviews.llvm.org/D14052
Files:
lib/Format/TokenAnnotator.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -8398,8 +8398,6 @@
verifyFormat("call( x, y, z );", Spaces);
verifyFormat("call();", Spaces);
verifyFormat("std::function callback;", Spaces);
- verifyFormat("void inFunction() { std::function fct; }",
- Spaces);
verifyFormat("while ( (bool)1 )\n"
" continue;",
Spaces);
@@ -10635,9 +10633,6 @@
verifyFormat("f< int, float >();", Spaces);
verifyFormat("template <> g() {}", Spaces);
verifyFormat("template < std::vector< int > > f() {}", Spaces);
- verifyFormat("std::function< void(int, int) > fct;", Spaces);
- verifyFormat("void inFunction() { std::function< void(int, int) > fct; }",
- Spaces);
Spaces.Standard = FormatStyle::LS_Cpp03;
Spaces.SpacesInAngles = true;
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -67,12 +67,6 @@
Left->MatchingParen = CurrentToken;
CurrentToken->MatchingParen = Left;
CurrentToken->Type = TT_TemplateCloser;
-if (CurrentToken->Previous->is(TT_CastRParen) &&
-CurrentToken->Previous->MatchingParen) {
- // Fix incorrect cast detection
- CurrentToken->Previous->Type = TT_Unknown;
- CurrentToken->Previous->MatchingParen->Type = TT_Unknown;
-}
next();
return true;
}
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -8398,8 +8398,6 @@
verifyFormat("call( x, y, z );", Spaces);
verifyFormat("call();", Spaces);
verifyFormat("std::function callback;", Spaces);
- verifyFormat("void inFunction() { std::function fct; }",
- Spaces);
verifyFormat("while ( (bool)1 )\n"
" continue;",
Spaces);
@@ -10635,9 +10633,6 @@
verifyFormat("f< int, float >();", Spaces);
verifyFormat("template <> g() {}", Spaces);
verifyFormat("template < std::vector< int > > f() {}", Spaces);
- verifyFormat("std::function< void(int, int) > fct;", Spaces);
- verifyFormat("void inFunction() { std::function< void(int, int) > fct; }",
- Spaces);
Spaces.Standard = FormatStyle::LS_Cpp03;
Spaces.SpacesInAngles = true;
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -67,12 +67,6 @@
Left->MatchingParen = CurrentToken;
CurrentToken->MatchingParen = Left;
CurrentToken->Type = TT_TemplateCloser;
-if (CurrentToken->Previous->is(TT_CastRParen) &&
-CurrentToken->Previous->MatchingParen) {
- // Fix incorrect cast detection
- CurrentToken->Previous->Type = TT_Unknown;
- CurrentToken->Previous->MatchingParen->Type = TT_Unknown;
-}
next();
Re: [PATCH] D14052: clang-format: Format std::function template parameter consistently inside function
jeanphilippeD added a comment. Thanks a lot. Yes, it makes a lot more sense this way. http://reviews.llvm.org/D14052 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16524: [clang-format-vs] Fix sort include main include: Use current path for the '-assume-filename'
jeanphilippeD added a comment. Ping, Hi Daniel, Sorry, should I have put someone else as reviewer, I see the previous committers for this file where Manuel Klimek, Hans Wennborg and Marek Kurdej. Should have i added any or all of them. I thought it made sense to put you originally since you updated the code for sort include in clang-format. Kind Regards, Jean-Philippe. http://reviews.llvm.org/D16524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16524: [clang-format-vs] Fix sort include main include: Use current path for the '-assume-filename'
jeanphilippeD added a comment. Hi Manuel, Thank you very much for the review. I do not have commit access, would you be able to commit this patch for me. Kind Regards, Jean-Philippe http://reviews.llvm.org/D16524 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r263709 - clang-format: Slightly weaken AlignAfterOpenBracket=AlwaysBreak.
Hi Daniel,
I'm not sure I understand the logic behind modifying 'AlwaysBreak' to be
'AlwaysBreakExceptForSingleArgument'.
AlwaysBreak is always going to lead to "wasted space".
Some consider consistently break to not be a waste but actually contribute
to readability, it seems that was what 'AlwaysBreak' was about.
Would you reconsider this change, or make it an option different from
'AlwaysBreak'?
We just completed the roll out clang-format at my company.
We are using: AlignAfterOpenBracket: AlwaysBreak
We tuned the settings to have a style that does not clash too much with our
original style, and that will look like a regression for us.
Kind regards,
Jean-Philippe.
2016-03-17 12:00 GMT+00:00 Daniel Jasper via cfe-commits <
cfe-commits@lists.llvm.org>:
> Author: djasper
> Date: Thu Mar 17 07:00:22 2016
> New Revision: 263709
>
> URL: http://llvm.org/viewvc/llvm-project?rev=263709&view=rev
> Log:
> clang-format: Slightly weaken AlignAfterOpenBracket=AlwaysBreak.
>
> If a call takes a single argument, using AlwaysBreak can lead to lots
> of wasted lines and additional indentation without improving the
> readability in a significant way.
>
> Before:
> cll(
> cll(
> cll(
> caaall(aa, a;
>
> After:
> cll(cll(cll(
> caaall(aa, a;
>
> Modified:
> cfe/trunk/lib/Format/ContinuationIndenter.cpp
> cfe/trunk/unittests/Format/FormatTest.cpp
>
> Modified: cfe/trunk/lib/Format/ContinuationIndenter.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/ContinuationIndenter.cpp?rev=263709&r1=263708&r2=263709&view=diff
>
> ==
> --- cfe/trunk/lib/Format/ContinuationIndenter.cpp (original)
> +++ cfe/trunk/lib/Format/ContinuationIndenter.cpp Thu Mar 17 07:00:22 2016
> @@ -356,7 +356,17 @@ void ContinuationIndenter::addTokenOnCur
>Previous.isOneOf(tok::l_paren, TT_TemplateOpener, tok::l_square) &&
>State.Column > getNewLineColumn(State) &&
>(!Previous.Previous ||
> - !Previous.Previous->isOneOf(tok::kw_for, tok::kw_while,
> tok::kw_switch)))
> + !Previous.Previous->isOneOf(tok::kw_for, tok::kw_while,
> + tok::kw_switch)) &&
> + // Don't do this for simple (no expressions) one-argument function
> calls
> + // as that feels like needlessly wasting whitespace, e.g.:
> + //
> + // cll(
> + // cll(
> + // cll(
> + // caaall(aa,
> a;
> + Current.FakeLParens.size() > 0 &&
> + Current.FakeLParens.back() > prec::Unknown)
> State.Stack.back().NoLineBreak = true;
>
>if (Style.AlignAfterOpenBracket != FormatStyle::BAS_DontAlign &&
>
> Modified: cfe/trunk/unittests/Format/FormatTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=263709&r1=263708&r2=263709&view=diff
>
> ==
> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
> +++ cfe/trunk/unittests/Format/FormatTest.cpp Thu Mar 17 07:00:22 2016
> @@ -4461,12 +4461,31 @@ TEST_F(FormatTest, AlignsAfterOpenBracke
> "aaa a,\n"
> "a);",
> Style);
> - verifyFormat("SomeLongVariableName->someFunction(\n"
> - "f(\n"
> - "aaa,\n"
> - "a,\n"
> - "a));",
> + verifyFormat("SomeLongVariableName->someFunction(f(\n"
> + "aaa,\n"
> + "a,\n"
> + "a));",
> Style);
> + verifyFormat(
> + "(a(\n"
> + "(a, )));",
> + Style);
> + verifyFormat(
> + "(aa.aa(\n"
> + "(a, )));",
> + Style);
> + verifyFormat(
> + "(\n"
> + "a(\n"
> + "(a,
> )),\n"
> + ");",
> + Style);
> + verifyFormat(
> + "(\n"
> + "a(\n"
> + "(a, ))
> &&\n"
> + ");",
> + Style);
> }
>
> TEST_F(FormatTest, ParenthesesAndOperandAlignment) {
>
>
> _
Re: r263709 - clang-format: Slightly weaken AlignAfterOpenBracket=AlwaysBreak.
Hi Daniel,
Thanks a lot for your answer and clarifications.
Sorry for the long answer, TLDR at the beginning (and end for code
examples).
To answer your 2 questions immediately:
- It happens a lot more than I thought, about 10% of the few thousands file
I checked had instances of this.
- Yes, there is a significant readability disadvantage as these functions
become a special case that needs to be read differently from all the other
functions.
With consistent AlwaysBreak, it is obvious from the shape of the call how
to read it (horizontally or diagonally).
Now, it is not longer obvious and it impact readability mostly for these
functions, but also a bit for the other functions since it is not clear if
they are one of the special case or not.
(The eye needs to jump at the end and then back to parse the arguments)
(This ls less/not of an issue for lambda and small functions)
My conclusion:
I really think an option should provide a consistent break but I'm really
not sure it should be a new option.
If I manage to convince you through the examples, that formatting all
function consistently will actually improve overall readability more
generally, I think AlwaysBreak is just fine as it was.
I'm happy to help with coding or writing tests for whatever decision you
make.
Examples:
I've pasted / attached the code examples that I saw, some many many times
(with .clang-format file).
You'll be able to see how before things were more consistent and now things
are moved to unexpected places depending on the call.
Moreover the benefit is very limited for us, because gaining 1 line does
not seem to add to our readability, and the indentation saved (most often 1
(4 spaces)) does not lead to better or different arguments formatting in
most cases. (less than 10 where arguments where improved in the hundreds of
cases I've looked at).
Methodology:
I wanted to make sure that I really understood the problem before coming
back to you:
- I've done a full reformat of 2 of our projects first with snapshot
r260967, then with snapshot r264047 (with the new change).
That is few thousands of cpp/h files, and about 10% were modified when
running the updated format.
- I spent more hours than I should have yesterday just scrolling through
and taking notes in a 30'000 line diff of the impact of the update (using
10 lines of context), so I think I've got a good handle on the issue now.
Other comments:
- It is possible that with some other configuration you used, this change
does improves things without the same draw back, I do not know, but your
commit comment example did not look more readable for me since I am used to
the style.
- lambda break would be nice and more what we would have expected, but they
are somewhat a different thing and less of a problem.
- Very small function break occurs not that often and are weird to format
either way. They are also less of an issue because they are small so
everything is mostly where the eye is looking at (no need for the eye to
jump toward the end of the line and back).
- I also think the new option as you described could have some trickiness,
it may also impact things like 'if' if we are not careful, and it is also
another maintenance burden.
Other notes / detailed explanations:
1 - With a consistent always break, there are 2 places you have too look
for, and the one your need is obvious from the shape of the code:
- Either it is multi-line, and 1st argument is described at the
indentation => Scan function call diagonally
- Or it is a single line call and the 1st argument is described after the
'(' => Scan function horizontally
With this new formatting, the 1st argument may also start to be described
after the '(' and then continue at the indentation.
It is no longer possible to scan diagonally, and that really apply for any
functions as it is not obvious from the shape of the call.
2 - It may be because of our other style options, but there are very few
(less than 10 in the many hundreds) where the result could be said to be
better, on its own:
This happens, because we most often only gain a new line (not really an
improvement for us) and only one indentation (4 spaces), and the odds that
the argument is too big by between 1 and 4 spaces is not so great:
So the net result is that the argument are shifted 4 spaces to the left
without really any improvements.
3- There is one unrelated bug with unstable comment re flowing in macros
that I found as a result and will try to raise it.
There was also some minor changes in the way few 'else if' were
formatted but this is not the main matter.
There was also an issue with formatting a template call with explicit
types as a function argument indented strangely.
Cheers,
Jean-Philippe
NewTest.cpp (also attached, contains both code reformatted with r26096 and
with r264047):
You can see how before things were more consistent and now things are moved
to unexpected places depending on the call.
class TestClass1
{
//
Re: r263709 - clang-format: Slightly weaken AlignAfterOpenBracket=AlwaysBreak.
Hi Daniel, Thanks, I understand why your users would have requested that and why you need to provide this improvement for them. I will survey the other people writing code with me on whether they are happy with this improvement considering both the readability of the statements and the relatively small impact in our day to day reading/writing. I'll get back to you, maybe I was wasting time on a non issue.. Cheers, Jean-Philippe. 2016-03-28 9:51 GMT+01:00 Daniel Jasper : > Hi Jean-Philippe, > > no, you have not convinced me with those examples. There is not a single > one, I find more readable. Many of them I find equally unreadable and would > write the code differently. But I would never have chose AlwaysBreak for my > own code in the first place and I rarely work in a codebase that uses it. > When I do, it usually bugs me. So, that (my opinion) is completely beside > the point. > > There is one fundamental half-way objective argument you can make here > (and in many other cases): Being more compact generally makes it easier to > read larger chunks of code as it is more likely for a whole > function/loop/... to fit on a single page. Using more linebreaks to make > the syntactic structure clearer and more uniform can make individual > statements more readable. My argument is that in this case, often many > lines are saved with very little to no impact on local readability. You > disagree and that's fine, I am not trying to convince you. People a > different here. > > If you need the uniformity so that many subsequent statements are > formatted equally (e.g. in your "connRegist.Add( ..." case), you are > likely doing it wrong. Try harder not to repeat yourself. The problem here > is that such uniformly appearing sequence can hide very subtle (copy&paste) > errors. However, even all of this is beside the point. > > You found changes in ~10% of your files. I'd bet that less than 10% of a > file was changed on average. So, this influences <1% of the code. That is > reasonably rare and you won't stumble over this frequently in practice. > Yes, of course you see hundreds of examples when you reformat a > thousand-file codebase and I understand why that bugs you. > > I definitely need the new behavior as it was requested frequently by my > internal users. This was the most frequent bug report I have gotten from > people using AlwaysBreak. I suggest that you also speak to the other > authors of the codebase you are working on to make sure that you are > actually getting a feel for what most people want here. And also include > cases where it is significantly better IMO: > > call(call(call(call(call(call(call(call(call(call(call( > parameterThatGoesUpToTheColumnLimit))); > > vs. > > call( > call( > call( > call( > ... > parameterThatGoesUpToTheColumnLimit))); // > probably even a column limit violation > > Again, I am not against adding an additional option. And if we do > introduce a new option, we should probably make that one strict and not > have the other exceptions. > > Cheers, > Daniel > > > On Sun, Mar 27, 2016 at 8:16 PM, Jean-philippe Dufraigne < > j.dufrai...@gmail.com> wrote: > >> Hi Daniel, >> >> Thanks a lot for your answer and clarifications. >> Sorry for the long answer, TLDR at the beginning (and end for code >> examples). >> >> To answer your 2 questions immediately: >> - It happens a lot more than I thought, about 10% of the few thousands >> file I checked had instances of this. >> - Yes, there is a significant readability disadvantage as these >> functions become a special case that needs to be read differently from all >> the other functions. >> With consistent AlwaysBreak, it is obvious from the shape of the call >> how to read it (horizontally or diagonally). >> Now, it is not longer obvious and it impact readability mostly for >> these functions, but also a bit for the other functions since it is not >> clear if they are one of the special case or not. >> (The eye needs to jump at the end and then back to parse the arguments) >> (This ls less/not of an issue for lambda and small functions) >> >> >> My conclusion: >> I really think an option should provide a consistent break but I'm really >> not sure it should be a new option. >> If I manage to convince you through the examples, that formatting all >> function consistently will actually improve overall readability more >> generally, I think AlwaysBreak is just fine as it was. >> I'm happy to help with coding or writing tests for whatever decision you >> make. >> >> >> Examples: >> I've pasted / attached the code examples that I saw, some many many times >> (with .clang-format file). >> You'll be able to see how before things were more consistent and now >> things are moved to unexpected places depending on the call. >> Moreover the benefit is very limited for us, because gaining 1 line does >> not seem to add to our readability, and th
Re: [PATCH] D15639: [clang-format] Ensure Sort include is stable with negative Priority
jeanphilippeD abandoned this revision. jeanphilippeD added a comment. This was fixed by other patch. http://reviews.llvm.org/D15639 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15266: [clang-format] Handle \n the same way as std::endl with stream operator.
jeanphilippeD added a comment. Ping. Is that patch of interest? http://reviews.llvm.org/D15266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15266: [clang-format] Handle \n the same way as std::endl with stream operator.
jeanphilippeD added a comment. Thanks, Would you be happy to commit it for me since i do not have commit access. http://reviews.llvm.org/D15266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D16524: [clang-format-vs] Fix sort include main include: Use current path for the '-assume-filename'
jeanphilippeD created this revision.
jeanphilippeD added a reviewer: djasper.
jeanphilippeD added a subscriber: cfe-commits.
clang-format sort include use the source file name to determine the "main
include" that will be the 1st include (category 0).
Because the clang-format visual studio extension does not pass the file name
and use the standard input, sort include cannot find a "main include":
Testing fix on llvm\tools\clang\lib\Format\Format.cpp:
Original file:
#include "clang/Format/Format.h"
...
#include "clang/Basic/SourceManager.h"
#include "clang/Lex/Lexer.h"
Without fix, selecting the includes and running visual studio clang-format:
...
#include "clang/Basic/SourceManager.h"
#include "clang/Format/Format.h"
#include "clang/Lex/Lexer.h"
With fix, selecting the includes and running visual studio clang-format:
#include "clang/Format/Format.h"
...
#include "clang/Basic/SourceManager.h"
#include "clang/Lex/Lexer.h"
Test 2 with main header not at the start:
Original file:
...
#include "clang/Format/Format.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Lex/Lexer.h"
Without fix, selecting the includes and running visual studio clang-format:
...
#include "clang/Basic/SourceManager.h"
#include "clang/Format/Format.h"
#include "clang/Lex/Lexer.h"
With fix, selecting the includes and running visual studio clang-format:
#include "clang/Format/Format.h"
...
#include "clang/Basic/SourceManager.h"
#include "clang/Lex/Lexer.h"
Caveat: My setup is not ideal for test (Visual studio 2015 only), and I am not
familiar with visual studio extension development, but I think the changes are
straight forward enough.
I've tested this fix building it and running it on Visual Studio 2015, but I do
not think there would be any compatibility issue since I am using API that are
already used in the GetDocumentParent function.
http://reviews.llvm.org/D16524
Files:
tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
Index: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
===
--- tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
+++ tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
@@ -202,9 +202,10 @@
if (start >= text.Length && text.Length > 0)
start = text.Length - 1;
string path = GetDocumentParent(view);
+string filePath = GetDocumentPath(view);
try
{
-var root = XElement.Parse(RunClangFormat(text, start, length,
path));
+var root = XElement.Parse(RunClangFormat(text, start, length,
path, filePath));
var edit = view.TextBuffer.CreateEdit();
foreach (XElement replacement in
root.Descendants("replacement"))
{
@@ -237,7 +238,7 @@
///
/// Formats the text range starting at offset of the given length.
///
-private string RunClangFormat(string text, int offset, int length,
string path)
+private string RunClangFormat(string text, int offset, int length,
string path, string filePath)
{
string vsixPath = Path.GetDirectoryName(
typeof(ClangFormatPackage).Assembly.Location);
@@ -257,6 +258,8 @@
if (GetSortIncludes())
process.StartInfo.Arguments += " -sort-includes ";
string assumeFilename = GetAssumeFilename();
+if (string.IsNullOrEmpty(assumeFilename))
+assumeFilename = filePath;
if (!string.IsNullOrEmpty(assumeFilename))
process.StartInfo.Arguments += " -assume-filename \"" +
assumeFilename + "\"";
process.StartInfo.CreateNoWindow = true;
@@ -355,5 +358,15 @@
}
return null;
}
+
+private string GetDocumentPath(IWpfTextView view)
+{
+ITextDocument document;
+if
(view.TextBuffer.Properties.TryGetProperty(typeof(ITextDocument), out document))
+{
+return document.FilePath;
+}
+return null;
+}
}
}
Index: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
===
--- tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
+++ tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
@@ -202,9 +202,10 @@
if (start >= text.Length && text.Length > 0)
start = text.Length - 1;
string path = GetDocumentParent(view);
+string filePath = GetDocumentPath(view);
try
{
-var root = XElement.Parse(RunClangFormat(text, start, length, path));
+var root = XElement.Parse(RunClangFormat(text, start, length, path, filePath));
var edit = view.TextBuffer.CreateEdit();
foreach (XElement replacement in root.Descendants("replacement"))
[PATCH] D15062: [clang-format] Add test for AlignAfterOpenBracket = AlwaysBreak in C++.
jeanphilippeD created this revision.
jeanphilippeD added a subscriber: cfe-commits.
Herald added a subscriber: klimek.
Revision 251405 added AlwaysBreak to support Google's JavaScript style.
This changeset complete existing AlignsAfterOpenBracket tests to exercise
AlwaysBreak for C++.
I thought this would be worthwhile.
With this option we can support request from
http://lists.llvm.org/pipermail/cfe-dev/2015-May/042942.html, that had been
requested a few times.
This also partially solve related Bug 23422 and is probably sufficient for most
people.
AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
BinPackArguments = false;
BinPackParameters = false;
With these setting we obtain this formatting:
void fooWithAVeryLongParamList(
int firstParameter,
int secondParameter
int lastParameter)
{
object.alsoThisDoenstFitSoIBreakImmidiatly(
firstParameter,
secondParameter,
lastParameter);
}
http://reviews.llvm.org/D15062
Files:
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -4344,6 +4344,26 @@
"SomeLongVariableName->someFunction(f(aaa,\n"
"a, a));",
Style);
+
+ Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+ Style.BinPackArguments = false;
+ Style.BinPackParameters = false;
+ verifyFormat("void aa(\n"
+ "aaa ,\n"
+ "a aaa,\n"
+ "a) {}",
+ Style);
+ verifyFormat("SomeLongVariableName->someVeryLongFunctionName(\n"
+ "aaa a,\n"
+ "aaa a,\n"
+ "a);",
+ Style);
+ verifyFormat("SomeLongVariableName->someFunction(\n"
+ "f(\n"
+ "aaa,\n"
+ "a,\n"
+ "a));",
+ Style);
}
TEST_F(FormatTest, ParenthesesAndOperandAlignment) {
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -4344,6 +4344,26 @@
"SomeLongVariableName->someFunction(f(aaa,\n"
"a, a));",
Style);
+
+ Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+ Style.BinPackArguments = false;
+ Style.BinPackParameters = false;
+ verifyFormat("void aa(\n"
+ "aaa ,\n"
+ "a aaa,\n"
+ "a) {}",
+ Style);
+ verifyFormat("SomeLongVariableName->someVeryLongFunctionName(\n"
+ "aaa a,\n"
+ "aaa a,\n"
+ "a);",
+ Style);
+ verifyFormat("SomeLongVariableName->someFunction(\n"
+ "f(\n"
+ "aaa,\n"
+ "a,\n"
+ "a));",
+ Style);
}
TEST_F(FormatTest, ParenthesesAndOperandAlignment) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15062: [clang-format] Add test for AlignAfterOpenBracket = AlwaysBreak in C++.
jeanphilippeD added a comment. Hi Daniel, Realized that I should have added you when I submitted it. Not sure if this review slipped through, or you were too busy, or if this change is not desired since the code was added to support JS. Kind Regards, Jean-Philippe. http://reviews.llvm.org/D15062 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15266: [clang-format] Handle \n the same way as std::endl with stream operator.
jeanphilippeD created this revision.
jeanphilippeD added a reviewer: djasper.
jeanphilippeD added a subscriber: cfe-commits.
Herald added a subscriber: klimek.
clang-format break multi-line streams after std::endl.
It now also break for '\n', the suggested replacement for std::endl:
http://llvm.org/docs/CodingStandards.html#avoid-std-endl
Before:
llvm::errs() << aa << '\n' << bb
<< '\n';
llvm::errs() << << "aa\n" <<
<< "bb\n";
After:
llvm::errs() << aa << '\n'
<< bb << '\n';
llvm::errs() << << "aa\n"
<< << "bb\n";
This changeset ensure that multiline streams have a line break after:
-std::endl
-'\n'
-"\n"
-"Some Text\n"
Test:
-Run unit tests
-Run clang format on Format.cpp without it making changes compared to current
clang-format.
http://reviews.llvm.org/D15266
Files:
lib/Format/ContinuationIndenter.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -4964,6 +4964,15 @@
verifyFormat("llvm::errs() << aa << endl\n"
" << bb << endl;");
verifyFormat("llvm::errs() << endl << bb << endl;");
+
+ // Handle '\n'.
+ verifyFormat("llvm::errs() << aa << \"\\n\"\n"
+ " << bb << \"\\n\";");
+ verifyFormat("llvm::errs() << aa << \'\\n\'\n"
+ " << bb << \'\\n\';");
+ verifyFormat("llvm::errs() << << \"aa\\n\"\n"
+ " << << \"bb\\n\";");
+ verifyFormat("llvm::errs() << \"\\n\" << bb <<
\"\\n\";");
}
TEST_F(FormatTest, UnderstandsEquals) {
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -246,8 +246,10 @@
Previous.is(tok::l_brace) && !Current.isOneOf(tok::r_brace,
tok::comment))
return true;
- if (Current.is(tok::lessless) && Previous.is(tok::identifier) &&
- Previous.TokenText == "endl")
+ if (Current.is(tok::lessless) &&
+ ((Previous.is(tok::identifier) && Previous.TokenText == "endl") ||
+ (Previous.Tok.isLiteral() && (Previous.TokenText.endswith("\\n\"") ||
+ Previous.TokenText == "\'\\n\'"
return true;
return false;
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -4964,6 +4964,15 @@
verifyFormat("llvm::errs() << aa << endl\n"
" << bb << endl;");
verifyFormat("llvm::errs() << endl << bb << endl;");
+
+ // Handle '\n'.
+ verifyFormat("llvm::errs() << aa << \"\\n\"\n"
+ " << bb << \"\\n\";");
+ verifyFormat("llvm::errs() << aa << \'\\n\'\n"
+ " << bb << \'\\n\';");
+ verifyFormat("llvm::errs() << << \"aa\\n\"\n"
+ " << << \"bb\\n\";");
+ verifyFormat("llvm::errs() << \"\\n\" << bb << \"\\n\";");
}
TEST_F(FormatTest, UnderstandsEquals) {
Index: lib/Format/ContinuationIndenter.cpp
===
--- lib/Format/ContinuationIndenter.cpp
+++ lib/Format/ContinuationIndenter.cpp
@@ -246,8 +246,10 @@
Previous.is(tok::l_brace) && !Current.isOneOf(tok::r_brace, tok::comment))
return true;
- if (Current.is(tok::lessless) && Previous.is(tok::identifier) &&
- Previous.TokenText == "endl")
+ if (Current.is(tok::lessless) &&
+ ((Previous.is(tok::identifier) && Previous.TokenText == "endl") ||
+ (Previous.Tok.isLiteral() && (Previous.TokenText.endswith("\\n\"") ||
+ Previous.TokenText == "\'\\n\'"
return true;
return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15062: [clang-format] Add test for AlignAfterOpenBracket = AlwaysBreak in C++.
jeanphilippeD added a comment. Thanks a lot, I do not have commit access. I'm happy for you to commit it when you have time. http://reviews.llvm.org/D15062 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15639: [clang-format] Ensure Sort include is stable with negative Priority
jeanphilippeD created this revision.
jeanphilippeD added a reviewer: djasper.
jeanphilippeD added a subscriber: cfe-commits.
Herald added a subscriber: klimek.
The new sort include with negative priority was not stable when running it
again over the updated includes.
Extend the test NegativePriorities to have an extra header. Test pas still
passing
Add a new assertion to test that the sorting is stable. This new assertion
failed.
From this:
#include "important_os_header.h"
#include "c_main_header.h"
#include "a_other_header.h"
Before fix:
#include "important_os_header.h"
#include "a_other_header.h"
#include "c_main_header.h"
After fix:
#include "important_os_header.h"
#include "c_main_header.h"
#include "a_other_header.h"
http://reviews.llvm.org/D15639
Files:
lib/Format/Format.cpp
unittests/Format/SortIncludesTest.cpp
Index: unittests/Format/SortIncludesTest.cpp
===
--- unittests/Format/SortIncludesTest.cpp
+++ unittests/Format/SortIncludesTest.cpp
@@ -188,9 +188,19 @@
TEST_F(SortIncludesTest, NegativePriorities) {
Style.IncludeCategories = {{".*important_os_header.*", -1}, {".*", 1}};
EXPECT_EQ("#include \"important_os_header.h\"\n"
-"#include \"a.h\"\n",
-sort("#include \"a.h\"\n"
+"#include \"c_main_header.h\"\n"
+"#include \"a_other_header.h\"\n",
+sort("#include \"c_main_header.h\"\n"
+ "#include \"a_other_header.h\"\n"
"#include \"important_os_header.h\"\n"));
+
+ // check stable when re-run
+ EXPECT_EQ("#include \"important_os_header.h\"\n"
+"#include \"c_main_header.h\"\n"
+"#include \"a_other_header.h\"\n",
+sort("#include \"important_os_header.h\"\n"
+ "#include \"c_main_header.h\"\n"
+ "#include \"a_other_header.h\"\n"));
}
TEST_F(SortIncludesTest, CalculatesCorrectCursorPosition) {
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -1807,19 +1807,21 @@
if (!FormattingOff && !Line.endswith("\\")) {
if (IncludeRegex.match(Line, &Matches)) {
StringRef IncludeName = Matches[2];
-int Category;
-if (LookForMainHeader && !IncludeName.startswith("<")) {
- Category = 0;
-} else {
- Category = INT_MAX;
- for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i) {
+int Category = INT_MAX;
+for (unsigned i = 0, e = CategoryRegexs.size(); i != e; ++i) {
if (CategoryRegexs[i].match(IncludeName)) {
- Category = Style.IncludeCategories[i].Priority;
- break;
+Category = Style.IncludeCategories[i].Priority;
+break;
}
- }
}
-LookForMainHeader = false;
+
+if (Category >= 0 ) {
+if (LookForMainHeader && !IncludeName.startswith("<")) {
+Category = 0;
+}
+LookForMainHeader = false;
+}
+
IncludesInBlock.push_back({IncludeName, Line, Prev, Category});
} else if (!IncludesInBlock.empty()) {
sortIncludes(Style, IncludesInBlock, Ranges, FileName, Replaces,
Index: unittests/Format/SortIncludesTest.cpp
===
--- unittests/Format/SortIncludesTest.cpp
+++ unittests/Format/SortIncludesTest.cpp
@@ -188,9 +188,19 @@
TEST_F(SortIncludesTest, NegativePriorities) {
Style.IncludeCategories = {{".*important_os_header.*", -1}, {".*", 1}};
EXPECT_EQ("#include \"important_os_header.h\"\n"
-"#include \"a.h\"\n",
-sort("#include \"a.h\"\n"
+"#include \"c_main_header.h\"\n"
+"#include \"a_other_header.h\"\n",
+sort("#include \"c_main_header.h\"\n"
+ "#include \"a_other_header.h\"\n"
"#include \"important_os_header.h\"\n"));
+
+ // check stable when re-run
+ EXPECT_EQ("#include \"important_os_header.h\"\n"
+"#include \"c_main_header.h\"\n"
+"#include \"a_other_header.h\"\n",
+sort("#include \"important_os_header.h\"\n"
+ "#include \"c_main_header.h\"\n"
+ "#include \"a_other_header.h\"\n"));
}
TEST_F(SortIncludesTest, CalculatesCorrectCursorPosition) {
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -1807,19 +1807,21 @@
if (!FormattingOff && !Line.endswith("\\")) {
if (IncludeRegex.match(Line, &Matches)) {
StringRef IncludeName = Matches[2];
-int Category;
-if (LookForMainHeader && !IncludeName.startswith("<")) {
- Category = 0;
-} else {
- Category = INT_MAX;
- for (unsigned i = 0, e = CategoryRe
