[PATCH] Calculate and store ColumnWidth instead of CodePointCount in FormatTokens.

Alexander Kornienko alexfh at google.com
Mon Sep 9 09:19:54 PDT 2013


  PTAL


================
Comment at: lib/Format/Format.cpp:696
@@ +695,3 @@
+    // FIXME: ColumnWidth actually depends on the start column, we need to take
+    // this to account when the token is moved.
+    FormatTok->ColumnWidth =
----------------
Daniel Jasper wrote:
> nit: .. into account ..
Done.

================
Comment at: lib/Format/Format.cpp:687
@@ +686,3 @@
+    if (FirstNewlinePos != StringRef::npos) {
+      // LastLineColumnWidth is always correct, as it always starts in the
+      // beginning of the line.
----------------
Daniel Jasper wrote:
> This comment is hard to understand. How about:
>  
>   // The last line of this token always starts in column 0.
>   // Thus, the length can be precomputed even in the presence of tabs.
Yep, though not perfect, this explain things better than the original comment.

================
Comment at: lib/Format/Format.cpp:962
@@ +961,3 @@
+    // Never try to merge lines with multiline tokens.
+    for (SmallVectorImpl<AnnotatedLine *>::iterator It = I; It != E; ++It) {
+      for (const FormatToken *Tok = (*It)->First; Tok; Tok = Tok->Next) {
----------------
Daniel Jasper wrote:
> This should not be necessary, as TotalLength will be set to something > TotalLength for multiline tokens. Can you create a test case that fails without this? If so, why?
I've added the check after a few tests started failing.

Without this check, this code:
void f() {
/* qqqqqqq
qqqqqqqq */
}

gets merged to a "single" line:
void f() { /* qqqqqqq
qqqqqqqq */ }

(the second line of the comment probably also gets indented, but I'm not sure).

================
Comment at: lib/Format/FormatToken.h:124
@@ -125,1 +123,3 @@
+  /// no newline in the token.
+  enum { NoNewline = UINT_MAX };
 
----------------
Daniel Jasper wrote:
> How about just using the LastLineColumnWidth 0 for this? I think that would be quite self-explanatory as the last line can never actually have the length 0.
It can, if a line comment ends with an escaped newline (there's now a test for this, btw).

OTOH LastLineColumnWidth is currently only used for string literals, so we could use an arbitrary non-zero value for multi-line line comments. We could also store the column + 1, and 0 would be really a dedicated value for the single line case. But from all these options I prefer the one currently implemented.


http://llvm-reviews.chandlerc.com/D1608



More information about the cfe-commits mailing list