================
Comment at: lib/Format/Format.cpp:935
@@ +934,3 @@
+ 1;
+ State.Column = Column;
+ return (Column > getColumnLimit())
----------------
Daniel Jasper wrote:
> Alexander Kornienko wrote:
> > Daniel Jasper wrote:
> > > I am confused by this. Shouldn't State.Column be set to the remaining
> > > length of the comment after the last escaped newline?
> > There's always a line break after the comment, and we don't adjust leading
> > whitespace in consecutive lines, so we don't need to care about anything
> > after the first escaped newline.
> >
> > Here I set State.Column to the last column of the first line to ensure
> > proper penalty added if we violate the column limit in the first line. This
> > is what is checked in FormatTest.cpp:938.
> Ok, but aren't you then adding this penalty twice? Once in line 936 below and
> once when coming out of this function?
Yep, you're right. Removed additional penalty.
================
Comment at: lib/Format/Format.cpp:937
@@ +936,3 @@
+ return (Column > getColumnLimit())
+ ? Style.PenaltyExcessCharacter * (Column - getColumnLimit())
+ : 0;
----------------
Daniel Jasper wrote:
> Alexander Kornienko wrote:
> > Daniel Jasper wrote:
> > > With this, you are adding the penalty for the first line (i.e. for the
> > > length of the comment up to the first escaped newline).
> > >
> > > It seems to me that we should either add the penalty for each of the
> > > comment's lines (except the last one, which is added later) or for none
> > > of them. Otherwise, this is a half-way solution that is only correct for
> > > a few edge cases and might confuse people.
> > As I've noted in my previous comment, we don't touch lines after the first
> > one, so we don't need to add penalty for them (it will be constant).
> It is constant until we decide we want to go ahead and shift all the
> comment's lines (possibly dependent on some other formatting - see your test
> case). However, I agree that this is not important now and should be
> constant. However, then I still don't get why you would need to add the
> penalty for the first line here.
For no reason. Removed.
================
Comment at: unittests/Format/FormatTest.cpp:938
@@ +937,3 @@
+ getLLVMStyleWithColumns(50)));
+ EXPECT_EQ("int\n"
+ "a; // AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\\\n"
----------------
Daniel Jasper wrote:
> Alexander Kornienko wrote:
> > Daniel Jasper wrote:
> > > Maybe add a FIXME as this is not generally desirable. I know that there
> > > is one in the code, but having that also in the test is also helpful.
> > > E.g. imagine you fix this some day (either directly or by accident doing
> > > another change). If you the find this comment on the breaking test, you
> > > know that you can just fix the test.
> > These two tests ensure that we calculate columns used by the comment's
> > first line correctly. There's nothing to fix here.
> I know, what the test is meant to do. But in the long run, we'd probably want
> to fix the indent of the other lines of the comment, no?
Ah, you mean adjusting indentation of other lines. Yes, this deserves a FIXME.
http://llvm-reviews.chandlerc.com/D1146
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
