[PATCH] clang-format: [js] Support template strings.

Martin Probst martinprobst at google.com
Fri Feb 20 04:17:50 PST 2015


================
Comment at: lib/Format/Format.cpp:796-797
@@ +795,4 @@
+      ++TokenCount;
+      IsMultiline =
+          IsMultiline || I[0]->NewlinesBefore > 0 || I[0]->IsMultiline;
+      if (I[0]->isNot(tok::unknown) || I[0]->TokenText != "`")
----------------
djasper wrote:
> I think, I'd find this (very slightly) easier to read as:
> 
>   if (I[0]->IsMultiline || I[0]->NewlinesBefore > 0)
>     IsMultiline = true;
Done.

================
Comment at: lib/Format/Format.cpp:798
@@ +797,3 @@
+          IsMultiline || I[0]->NewlinesBefore > 0 || I[0]->IsMultiline;
+      if (I[0]->isNot(tok::unknown) || I[0]->TokenText != "`")
+        continue;
----------------
djasper wrote:
> Can you add a test with two template strings? I think that might do the wrong thing as you need to abort when you find a TT_TemplateString.
It's not strictly needed - if there was a preceding template string, it cannot just equals "`", it must have at least opener and closer "``". But it's a reasonable optimization and IMHO adds clarity.

================
Comment at: lib/Format/Format.cpp:811
@@ +810,3 @@
+            SourceMgr.getSpellingColumnNumber(End->Tok.getLocation()) + 1;
+        Tokens.back()->ColumnWidth = EndCol - StartCol;
+      }
----------------
djasper wrote:
> Reading the comment for ColumnWidth again, it says:
> 
>   /// \brief The width of the non-whitespace parts of the token (or its first
>   /// line for multi-line tokens) in columns.
> 
> So, if this turns into a multiline token, we need the length of the first line. The reason is this:
> 
>   var someLongVariableName = `aaaaaaaaa
>           bbbbbbb
>           cccccccc`;
> 
> While we can't do anything to the content of the template string without affecting the runtime behavior, we need to make the decision of whether or not to break after the "=". For that, we need to know the lengths of the first line of the token.
> 
> Should be reasonably easy to reproduce in a test :-).
As discussed offline, this changed quite a bit. PTAL.

================
Comment at: lib/Format/Format.cpp:813
@@ +812,3 @@
+      }
+      Tokens.back()->LastLineColumnWidth = LastColumn;
+      Tokens.back()->IsMultiline = IsMultiline;
----------------
djasper wrote:
> I think we need to set LastLineColumnWidth only for MultiLine tokens. It should not have any meaning for single-line tokens. Setting it to LastColumn for non-multiline tokens certainly is confusing.
Done.

http://reviews.llvm.org/D7763

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the cfe-commits mailing list