curdeius added a comment.
I agree with both of you. I shouldn't have used the word "regression" indeed. I
just meant a change in behaviour. Sorry for that.
I'll try to play around and propose a patch to enhance this feature :).
If you have any pointers about how to check if everything fits on a s
MyDeveloperDay added a comment.
@oleg.smolsky I agree, what you have here covers a myriad of other cases and
was committed in 2018, we can't call this commit a regression it was a feature
;-), if we want to improve the feature that is something else.
Repository:
rL LLVM
CHANGES SINCE LAST
oleg.smolsky added a comment.
@MyDeveloperDay that's the exact point. I authored this change to close a gap
in some lambda formatting cases. The tests (existing, modified and added)
express all relevant cases that I knew at the time.
@jaafar, @curdeius - this is just C++ code. Nothing is set in
MyDeveloperDay added a comment.
I think if you have a proposal for changing the behavior lets see the patch and
how it impacts the existing unit tests
something tells me these tests are going to change?
// A lambda passed as arg0 is always pushed to the next line.
verifyFormat("SomeFunction
curdeius added subscribers: MyDeveloperDay, curdeius.
curdeius added a comment.
Hi,
I know it's an old revision, but I confirm that it provoked the bug
https://bugs.llvm.org/show_bug.cgi?id=45141.
The problem is in the `TokenAnnotator::mustBreakBefore` as @jaafar pointed out:
if (!Next->isOneO
jaafar added inline comments.
Comment at: cfe/trunk/lib/Format/TokenAnnotator.cpp:3072
+ return false;
+if (!Next->isOneOf(TT_LambdaLSquare, tok::l_brace, tok::caret))
+ return true;
This is the clause that triggers the problem.
Repository:
rL L
jaafar added a comment.
It's been pointed out to me that 28546 preceded this commit... so it may only
//appear// related.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D52676/new/
https://reviews.llvm.org/D52676
___
c
jaafar added a comment.
Hi everyone, I've been investigating a bug and bisected it to this commit.
The problem, in brief, is that lambdas passed as arguments can cause an extra
line break before the first argument, but only if the lambda is neither the
first nor the last argument. This implies,
This revision was automatically updated to reflect the committed changes.
Closed by commit rL345753: [clang-format] tweaked another case of lambda
formatting (authored by krasimir, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D52676?
krasimir accepted this revision.
krasimir added a comment.
This revision is now accepted and ready to land.
Thank you!
Repository:
rC Clang
https://reviews.llvm.org/D52676
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.
oleg.smolsky added a comment.
In https://reviews.llvm.org/D52676#1282335, @krasimir wrote:
> Looks good! Will stamp when the scopes are removed.
Cool, thanks, Krasimir. I've just posted the updated patch.
> Oleg, do you need someone to commit this for you?
I do, could you commit this please?
oleg.smolsky updated this revision to Diff 171932.
oleg.smolsky marked 2 inline comments as done.
Repository:
rC Clang
https://reviews.llvm.org/D52676
Files:
lib/Format/ContinuationIndenter.cpp
lib/Format/FormatToken.h
lib/Format/TokenAnnotator.cpp
lib/Format/UnwrappedLineFormatter.cpp
krasimir added a comment.
Looks good! Will stamp when the scopes are removed. Oleg, do you need someone
to commit this for you?
Repository:
rC Clang
https://reviews.llvm.org/D52676
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://l
oleg.smolsky marked 2 inline comments as done.
oleg.smolsky added inline comments.
Comment at: unittests/Format/FormatTest.cpp:11854
+ // case above.
+ {
+auto Style = getGoogleStyle();
djasper wrote:
> No need to use a scope here. Feel free to redefine Sty
djasper added a comment.
I think this roughly looks fine. Krasimir, any thoughts?
Comment at: unittests/Format/FormatTest.cpp:11854
+ // case above.
+ {
+auto Style = getGoogleStyle();
No need to use a scope here. Feel free to redefine Style.
If in fact
oleg.smolsky added a comment.
Folks, are there any other comments/suggestions?
Repository:
rC Clang
https://reviews.llvm.org/D52676
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
oleg.smolsky updated this revision to Diff 170915.
oleg.smolsky added a comment.
Added a FIXME comment at Krasimir's request.
Repository:
rC Clang
https://reviews.llvm.org/D52676
Files:
lib/Format/ContinuationIndenter.cpp
lib/Format/FormatToken.h
lib/Format/TokenAnnotator.cpp
lib/For
oleg.smolsky added inline comments.
Comment at: unittests/Format/FormatTest.cpp:11736
+ // line and there are no further args.
+ verifyFormat("function(1, [this, that] {\n"
+ " //\n"
krasimir wrote:
> oleg.smolsky wrote:
> > djasper wrote:
> > >
krasimir added inline comments.
Comment at: unittests/Format/FormatTest.cpp:11736
+ // line and there are no further args.
+ verifyFormat("function(1, [this, that] {\n"
+ " //\n"
oleg.smolsky wrote:
> djasper wrote:
> > oleg.smolsky wrote:
> > >
oleg.smolsky updated this revision to Diff 170776.
oleg.smolsky added a comment.
Corrected test regressions, removed temporary hacks.
Repository:
rC Clang
https://reviews.llvm.org/D52676
Files:
lib/Format/ContinuationIndenter.cpp
lib/Format/FormatToken.h
lib/Format/TokenAnnotator.cpp
oleg.smolsky added inline comments.
Comment at: clang/unittests/Format/FormatTest.cpp:78
EXPECT_EQ(Expected.str(), format(Code, Style));
+#if 0
if (Style.Language == FormatStyle::LK_Cpp) {
Oops., let me fix this...
Repository:
rC Clang
https://revi
oleg.smolsky updated this revision to Diff 170773.
oleg.smolsky added a comment.
Corrected test regressions.
Repository:
rC Clang
https://reviews.llvm.org/D52676
Files:
clang/lib/Format/ContinuationIndenter.cpp
clang/lib/Format/FormatToken.h
clang/lib/Format/TokenAnnotator.cpp
clang/
oleg.smolsky marked 2 inline comments as done.
oleg.smolsky added inline comments.
Comment at: unittests/Format/FormatTest.cpp:11604
+ " x.end(), //\n"
+ " [&](int, int) { return 1; });\n"
"}\n");
oleg.smols
oleg.smolsky marked 2 inline comments as done.
oleg.smolsky added inline comments.
Comment at: unittests/Format/FormatTest.cpp:11604
+ " x.end(), //\n"
+ " [&](int, int) { return 1; });\n"
"}\n");
djasper wr
djasper added inline comments.
Comment at: unittests/Format/FormatTest.cpp:11604
+ " x.end(), //\n"
+ " [&](int, int) { return 1; });\n"
"}\n");
oleg.smolsky wrote:
> krasimir wrote:
> > This looks a bit sus
oleg.smolsky updated this revision to Diff 170515.
oleg.smolsky edited the summary of this revision.
oleg.smolsky added a comment.
Added another test case.
Repository:
rC Clang
https://reviews.llvm.org/D52676
Files:
lib/Format/ContinuationIndenter.cpp
lib/Format/FormatToken.h
lib/Forma
oleg.smolsky added inline comments.
Comment at: unittests/Format/FormatTest.cpp:11604
+ " x.end(), //\n"
+ " [&](int, int) { return 1; });\n"
"}\n");
krasimir wrote:
> This looks a bit suspicious: I'd expect
krasimir added inline comments.
Comment at: unittests/Format/FormatTest.cpp:11604
+ " x.end(), //\n"
+ " [&](int, int) { return 1; });\n"
"}\n");
This looks a bit suspicious: I'd expect a break before the fi
oleg.smolsky updated this revision to Diff 170270.
oleg.smolsky added a comment.
Generalized the patch so that it deals with lambda args irrespective of bin
packing. Added additional tests and patched existing ones.
Repository:
rC Clang
https://reviews.llvm.org/D52676
Files:
lib/Format/Co
oleg.smolsky added a comment.
In https://reviews.llvm.org/D52676#1268806, @djasper wrote:
> In https://reviews.llvm.org/D52676#1268748, @oleg.smolsky wrote:
>
> > In https://reviews.llvm.org/D52676#1268706, @djasper wrote:
> >
> > > Ok, I think I agree with both of you to a certain extent, but I
djasper added a comment.
In https://reviews.llvm.org/D52676#1268748, @oleg.smolsky wrote:
> In https://reviews.llvm.org/D52676#1268706, @djasper wrote:
>
> > Ok, I think I agree with both of you to a certain extent, but I also think
> > this change as is, is not yet right.
> >
> > First, it do
oleg.smolsky added a comment.
In https://reviews.llvm.org/D52676#1268706, @djasper wrote:
> Ok, I think I agree with both of you to a certain extent, but I also think
> this change as is, is not yet right.
>
> First, it does too much. The original very first example in this CL is
> actually not
djasper added a comment.
Ok, I think I agree with both of you to a certain extent, but I also think this
change as is, is not yet right.
First, it does too much. The original very first example in this CL is actually
not produced by clang-format (or if it is, I don't know with which flag
combi
oleg.smolsky added a comment.
@djasper @klimek could you chime in please? This patch strives to cleanup a
quirk in lambda formatting which pushes code too far to the right. Here is the
problematic case:
void f() {
something.something.something.Method(some_arg,
krasimir added a comment.
In https://reviews.llvm.org/D52676#1251391, @oleg.smolsky wrote:
> In https://reviews.llvm.org/D52676#1251342, @krasimir wrote:
>
> > Digging a bit further, seems like the behavior you're looking for could be
> > achieved by setting the `AlignAfterOpenBracket` option to
oleg.smolsky added a comment.
In https://reviews.llvm.org/D52676#1251342, @krasimir wrote:
> Digging a bit further, seems like the behavior you're looking for could be
> achieved by setting the `AlignAfterOpenBracket` option to `DontAlign` or
> `AlwaysBreak`:
>
> % clang-format -style='{Based
krasimir added a comment.
Digging a bit further, seems like the behavior you're looking for could be
achieved by setting the `AlignAfterOpenBracket` option to `DontAlign` or
`AlwaysBreak`:
% clang-format -style='{BasedOnStyle: google, AlignAfterOpenBracket:
AlwaysBreak}' test.cc
void f() {
oleg.smolsky added a comment.
In https://reviews.llvm.org/D52676#1250124, @oleg.smolsky wrote:
> In https://reviews.llvm.org/D52676#1250071, @krasimir wrote:
>
> > In https://reviews.llvm.org/D52676#1249828, @oleg.smolsky wrote:
> >
> > > In https://reviews.llvm.org/D52676#1249784, @krasimir wrot
krasimir added a comment.
In https://reviews.llvm.org/D52676#1250124, @oleg.smolsky wrote:
> In https://reviews.llvm.org/D52676#1250071, @krasimir wrote:
>
> > In https://reviews.llvm.org/D52676#1249828, @oleg.smolsky wrote:
> >
> > > In https://reviews.llvm.org/D52676#1249784, @krasimir wrote:
>
oleg.smolsky added a comment.
In https://reviews.llvm.org/D52676#1250071, @krasimir wrote:
> In https://reviews.llvm.org/D52676#1249828, @oleg.smolsky wrote:
>
> > In https://reviews.llvm.org/D52676#1249784, @krasimir wrote:
> >
> > > IMO `BinPackArguments==false` does not imply that there should
oleg.smolsky updated this revision to Diff 167611.
oleg.smolsky marked an inline comment as done.
oleg.smolsky added a comment.
Tweaked if/else/return structure, added comments. No functional changes.
Repository:
rC Clang
https://reviews.llvm.org/D52676
Files:
lib/Format/ContinuationIndent
krasimir added a comment.
In https://reviews.llvm.org/D52676#1249828, @oleg.smolsky wrote:
> In https://reviews.llvm.org/D52676#1249784, @krasimir wrote:
>
> > IMO `BinPackArguments==false` does not imply that there should be a line
> > break before the first arguments, only that there should no
JonasToth added a comment.
Please upload the patch with full context (i belive `diff -c 999`)
Comment at: lib/Format/TokenAnnotator.cpp:3054
+ return true;
+else if (!Style.BinPackArguments)
+ return true;
please no `else` after return. You ca
43 matches
Mail list logo
