================ Comment at: lib/Format/Format.cpp:226 @@ +225,3 @@ + void splitLineInComment(const FormatToken &Tok, StringRef Line, + size_t ColumnLimit, const StringRef LinePrefix, + bool InPPDirective, const FormatStyle &Style, ---------------- Daniel Jasper wrote: > Don't hand in the column limit as it directly follows from \c > LinePrefix.size(), \c Style and \c InPPDirective (if I am not mistaken). > Instead, move (parts of) the calculation of the limit into a function > getColumnLimit.. I moved calculation to this function. I'm not sure it deserves a separate function (and the existing one is in a different class and does a bit different thing).
================ Comment at: lib/Format/Format.cpp:311 @@ +310,3 @@ + if (InPPDirective) + --ColumnLimit; // Reserve space for terminating backslash. + ---------------- Daniel Jasper wrote: > Another point for reusing more stuff. We have done the same before, but have > then decided to always leave at least one space before "\". So, this should > be ColumnLimit -= 2. Or, even better, find a way to reuse getColumnLimit().. Moved this to splitLineInComment, changed to -2. I don't see a good way to reuse getColumnLimit here. ================ Comment at: lib/Format/Format.cpp:280 @@ +279,3 @@ + break; + StringRef FirstPiece = Line.substr(0, SpacePos).rtrim(); + Line = Line.substr(SpacePos).ltrim(); ---------------- Daniel Jasper wrote: > Alexander Kornienko wrote: > > Daniel Jasper wrote: > > > maybe s/FirstPiece/BeforeSpace/ > > > > > > Does rtrim() actually change the behavior? I would have thought that line > > > 277 already ensures finding the first space ... > > I think FirstPiece is more suitable here. > > > > Line 275 finds the last space before ColumnLimit, so we need to find the > > first one. In case line 277 gets executed, rtrim doesn't make a difference. > And I disagree on that one. Especially, because it is not always the first > piece of the line. > > So rtrim is used, if there are spaces at the end of the line, but not before > the column limit? Does a test break if you remove it? > And I disagree on that one. Especially, because it is not always the first > piece of the line. How about NextCut? > So rtrim is used, if there are spaces at the end of the line, but not before > the column limit? Does a test break if you remove it? Yes, I have tests for this. They break, if I remove rtrim. http://llvm-reviews.chandlerc.com/D547 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
