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

Daniel Jasper djasper at google.com
Mon Sep 9 10:14:29 PDT 2013



================
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) {
----------------
Alexander Kornienko wrote:
> 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).
That's a bug in tryMergeSimpleBlock then. Lets fix that instead.

================
Comment at: lib/Format/FormatToken.h:124
@@ -125,1 +123,3 @@
+  /// no newline in the token.
+  enum { NoNewline = UINT_MAX };
 
----------------
Alexander Kornienko wrote:
> 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.
Or we do the sane thing and field and store an extra bool value. I don't like this UINT_MAX, because it is too easy to accidentally add something to it and get an unexpected value. Also, I am slightly worried about assuming that enums can always store UINT_MAX in a defined way.


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



More information about the cfe-commits mailing list