This is an archive of the discontinued LLVM Phabricator instance.

clang-format: [JS] wrap params with trailing commas.
ClosedPublic

Authored by mprobst on May 9 2017, 5:36 PM.

Details

Summary

JavaScript allows parameter lists to include trailing commas:

myFunction(param1, param2,);

For symmetry with other parenthesized lists ([...], {...}), clang-format
should wrap parenthesized lists one-per-line if they contain a trailing
comma:

myFunction(
    param1,
    param2,
);

This is particularly useful in function declarations or calls with many
arguments, e.g. commonly in constructors.

Diff Detail

Repository
rL LLVM

Event Timeline

mprobst created this revision.May 9 2017, 5:36 PM

Daniel - the tests here are currently failing, and I don't really understand why - it seems the BreakBeforeParameter is not taking effect?

mprobst updated this revision to Diff 98386.May 9 2017, 7:04 PM
  • get wrapping to work, but not quite
djasper added inline comments.May 15 2017, 1:41 AM
lib/Format/ContinuationIndenter.cpp
858 ↗(On Diff #98386)

Seems weird to have a newline here but not before l. 866.

871 ↗(On Diff #98386)

I think you'll have to use getPreviousNonComment() here. While generally we don't do this everywhere, this is a place where people are somewhat likely to put a comment.

mprobst updated this revision to Diff 98961.May 15 2017, 2:10 AM
  • closer to working code
mprobst updated this revision to Diff 98962.May 15 2017, 2:19 AM
  • fixed indentation of trailing close paren
  • test cases
mprobst updated this revision to Diff 98963.May 15 2017, 2:30 AM
  • fix various failing tests
djasper added inline comments.May 15 2017, 2:41 AM
lib/Format/ContinuationIndenter.cpp
1043 ↗(On Diff #98963)

Please make this specific to JavaScript for now. C++ doesn't allow trailing commas here and a trailing comma is more likely to represent "not done yet". In those cases, we don't want to reformat the whole call.

The part above (changing the indentation of a ")" wrapped to a new line is fine to be used for all languages).

unittests/Format/FormatTest.cpp
5505 ↗(On Diff #98963)

Don't add this line break? I suspect your clang-format isn't set to LLVM style?

9645 ↗(On Diff #98963)

What caused this change?

mprobst updated this revision to Diff 98969.May 15 2017, 3:20 AM
mprobst marked 2 inline comments as done.
  • only trigger wrapping in js mode, revert appropriate tests
mprobst marked an inline comment as done.May 15 2017, 3:21 AM
mprobst added inline comments.
unittests/Format/FormatTest.cpp
5505 ↗(On Diff #98963)

Yeah, something is amiss. Not sure what yet, but formatted manually for the time being.

9645 ↗(On Diff #98963)

The trailing comma triggers a break, and then the code in TokenAnnotator causes the wrapping logic (on trailing comma and trailing comment).

Only triggering the wrapping in JS mode, this now just unindents the closing paren, which I think is sensible.

mprobst updated this revision to Diff 98971.May 15 2017, 3:26 AM
mprobst marked an inline comment as done.

fix formatting a bit

mprobst edited the summary of this revision. (Show Details)May 15 2017, 4:07 AM
djasper accepted this revision.May 15 2017, 4:21 AM
This revision is now accepted and ready to land.May 15 2017, 4:21 AM
This revision was automatically updated to reflect the committed changes.