Page MenuHomePhabricator

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

Authored by mprobst on Feb 19 2015, 9:49 AM.

Details

Reviewers
djasper
Summary

Merge template strings (marked by backticks ``).
Do not format any contents of template strings.

Diff Detail

Event Timeline

mprobst updated this revision to Diff 20309.Feb 19 2015, 9:49 AM
mprobst retitled this revision from to clang-format: [js] Support template strings..
mprobst updated this object.
mprobst edited the test plan for this revision. (Show Details)
mprobst added a reviewer: djasper.
mprobst added subscribers: Unknown Object (MLST), klimek.
djasper added inline comments.Feb 20 2015, 12:09 AM
lib/Format/Format.cpp
796–797

I think, I'd find this (very slightly) easier to read as:

if (I[0]->IsMultiline || I[0]->NewlinesBefore > 0)
  IsMultiline = true;
811

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 :-).

813

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.

djasper added inline comments.Feb 20 2015, 12:12 AM
lib/Format/Format.cpp
798

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.

mprobst updated this revision to Diff 20389.Feb 20 2015, 4:16 AM

Fix counting of ColumnWidth and LastLineColumnWidth.

Still contains an issue with width counting of tok::unknown tokens.

mprobst added inline comments.Feb 20 2015, 4:17 AM
lib/Format/Format.cpp
796–797

Done.

798

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.

811

As discussed offline, this changed quite a bit. PTAL.

813

Done.

djasper accepted this revision.Feb 20 2015, 4:38 AM
djasper edited edge metadata.

Looks good, I'll add the test and commit.

lib/Format/Format.cpp
798

I agree. Still seems useful to have a test ;-).

This revision is now accepted and ready to land.Feb 20 2015, 4:38 AM
mprobst added inline comments.Feb 20 2015, 4:43 AM
lib/Format/Format.cpp
798

Sorry for being unclear, I actually did add one.

djasper closed this revision.Feb 20 2015, 5:49 AM

Submitted as r230011.