[PATCH] Split long lines in multi-line comments.
Daniel Jasper
djasper at google.com
Mon Mar 18 02:11:26 PDT 2013
================
Comment at: unittests/Format/FormatTest.cpp:661
@@ +660,3 @@
+ format("/*\n"
+ "This is a long "
+ "comment that doesn't "
----------------
nit: Is there a specific reason to split here and in the next line? I think not, but it makes you wonder a little.
================
Comment at: unittests/Format/FormatTest.cpp:658
@@ +657,3 @@
+ "comment that doesn't\n"
+ "fit on one line. \n"
+ "*/",
----------------
I think we should also delete trailing whitespace anywhere within block comments (we do that in line comments). Probably best to do it in a follow-up patch, though.
================
Comment at: lib/Format/Format.cpp:210
@@ +209,3 @@
+
+ bool safeToSplitLine(StringRef Line) {
+ return Line.find_first_of("/\\:<>()[]{}*&%") == StringRef::npos;
----------------
nit: I'd prefer isLineSafeToSplit
================
Comment at: lib/Format/Format.cpp:196
@@ -195,2 +195,3 @@
private:
- void indentBlockComment(const FormatToken &Tok, int Indent) {
+ StringRef findCommonPrefix(ArrayRef<StringRef> Lines) {
+ if (Lines.size() < 3)
----------------
This needs a comment describing the logic (or input->output examples).
================
Comment at: lib/Format/Format.cpp:201
@@ +200,3 @@
+ for (size_t i = 2; i < Lines.size(); ++i) {
+ for (size_t j = 0; j < Prefix.size() && j < Lines[i].size(); ++j)
+ if (Prefix[j] != Lines[i][j]) {
----------------
Nit: Add {}
================
Comment at: lib/Format/Format.cpp:254
@@ +253,3 @@
+ if (Prefix.endswith("*")) {
+ NewLine += " ";
+ --ColumnLimit;
----------------
I presume, that the " " is not part of the prefix because findCommonPrefix also looks at the last line of a comment which can be "*/". Why can't findCommonPrefix not just ignore that line instead?
================
Comment at: lib/Format/Format.cpp:274
@@ +273,3 @@
+ SourceMgr,
+ TokenLoc.getLocWithOffset(Line.data() + BreakPos - Text.data()),
+ SpacePos - BreakPos + 1, NewLine));
----------------
I am a bit worried that this seems to rely on implementation details of StringRef (with data() always returning the exact pointer offset after calls to split, substr and ltrim).
================
Comment at: lib/Format/Format.cpp:246
@@ +245,3 @@
+ StringRef Prefix = findCommonPrefix(Lines);
+ size_t ColumnLimit = Style.ColumnLimit - Prefix.size() - IndentDelta;
+ std::string NewLine("\n");
----------------
Try to reuse more stuff from breakProtrudingToken(), which does something similar for String literals. Especially, reuse WhitespaceManager::breakToken(), which already does some of this logic and also works in multi-line defines.
http://llvm-reviews.chandlerc.com/D547
More information about the cfe-commits
mailing list