Page MenuHomePhabricator

clang-format: [JS] options for arrow functions.
ClosedPublic

Authored by mprobst on Jan 24 2020, 2:34 AM.

Details

Summary

clang-format currently always wraps the body of non-empty arrow
functions:

const x = () => {
  z();
};

This change implements support for the AllowShortLambdasOnASingleLine
style options, and also sets the default Google style to SLS_All, i.e.
to indent arrow function bodies that have one or fewer statements on a
single line:

const x = () => { z(); };

Multi-statement arrow functions continue to be wrapped. Function
expressions (a = function() {}) and function/method declarations are
unaffected as well.

Diff Detail

Event Timeline

mprobst created this revision.Jan 24 2020, 2:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2020, 2:34 AM

FYI I've started a conversation on whether SLS_All should be the default Google style. But the option I think is useful in either case.

FYI I've started a conversation on whether SLS_All should be the default Google style. But the option I think is useful in either case.

If that's the case, consider adding the option to the style in a follow-up patch:

GoogleStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;

that would have an advantage of making clear which test case diffs are result of the implementation and which are result of the style flip.

Eugene.Zelenko added a project: Restricted Project.
krasimir requested changes to this revision.Jan 24 2020, 7:44 AM
This revision now requires changes to proceed.Jan 24 2020, 7:44 AM

If that's the case, consider adding the option to the style in a follow-up patch:

GoogleStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;

that would have an advantage of making clear which test case diffs are result of the implementation and which are result of the style flip.

Done!

mprobst updated this revision to Diff 240202.Jan 24 2020, 7:49 AM
  • Disable arrow functions on single lines by default.
krasimir accepted this revision.Jan 24 2020, 8:05 AM
krasimir added inline comments.
clang/unittests/Format/FormatTestJS.cpp
1793

we probably want to keep the empty body test case.

This revision is now accepted and ready to land.Jan 24 2020, 8:05 AM
MyDeveloperDay accepted this revision.Jan 27 2020, 6:44 AM
This revision was automatically updated to reflect the committed changes.
RKSimon added inline comments.
clang/lib/Format/TokenAnnotator.cpp
3165

@mprobst - this is breaking buildbots: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-android/builds/27196/steps/bootstrap%20clang/logs/stdio

Please can you remove the default case and add a suitable llvm_unreachable() after the switch statement ?

mprobst marked 2 inline comments as done.Jan 28 2020, 6:04 AM
mprobst added inline comments.
clang/lib/Format/TokenAnnotator.cpp
3165

Sorry for the breakage, and thanks to Jinsong Ji for fixing while I was OOO.