[PATCH] Split long lines in multi-line comments.

Alexander Kornienko alexfh at google.com
Tue Mar 19 07:57:25 PDT 2013



================
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



More information about the cfe-commits mailing list