================
Comment at: lib/Format/Format.cpp:937
@@ +936,3 @@
+ return (Column > getColumnLimit())
+ ? Style.PenaltyExcessCharacter * (Column - getColumnLimit())
+ : 0;
----------------
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.
================
Comment at: lib/Format/Format.cpp:935
@@ +934,3 @@
+ 1;
+ State.Column = Column;
+ return (Column > getColumnLimit())
----------------
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?
================
Comment at: unittests/Format/FormatTest.cpp:938
@@ +937,3 @@
+ getLLVMStyleWithColumns(50)));
+ EXPECT_EQ("int\n"
+ "a; // AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\\\n"
----------------
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?
http://llvm-reviews.chandlerc.com/D1146
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
