This is an archive of the discontinued LLVM Phabricator instance.

clang-format: [JS] generator and async functions.
ClosedPublic

Authored by mprobst on Apr 17 2016, 5:00 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mprobst updated this revision to Diff 54015.Apr 17 2016, 5:00 PM
mprobst retitled this revision from to clang-format: [JS] generator and async functions..
mprobst updated this object.
mprobst added a reviewer: djasper.
mprobst updated this revision to Diff 54017.Apr 17 2016, 5:09 PM
  • fix tests
  • add spec links
djasper added inline comments.Apr 19 2016, 5:35 AM
unittests/Format/FormatTestJS.cpp
339 ↗(On Diff #54017)

What does the star mean here? Should we actually introduce a token type or do you think it's just going to work? Specifically, what happens if you need to line-wrap?

mprobst updated this revision to Diff 54195.Apr 19 2016, 8:21 AM
  • test for wrapping
mprobst marked an inline comment as done.Apr 19 2016, 8:22 AM
mprobst added inline comments.
unittests/Format/FormatTestJS.cpp
339 ↗(On Diff #54017)

The star indicates that this is a function that returns a generator, i.e. implicitly returns an iterator in which you shove values with yield. I think you wouldn't want to wrap between function and *. We currently don't, but more by chance - it's a side effect of not breaking before a binary operator.

function* cannot be one token, there may be whitespace and comments in between. We could have a generator star token, but there are places where that'd be hard for us to detect, e.g. the class method above. Having a generator star only occasionally be recognized of course would be rather confusing.

So I'd punt on this for the time beimg. I've added a test to make sure we correctly wrap above. Does that make sense?

djasper added inline comments.Apr 19 2016, 11:27 AM
unittests/Format/FormatTestJS.cpp
339 ↗(On Diff #54195)

Fundamentally, I think there is two things we might need to do:

  1. Not allow wrapping it. I am ok with keeping that based on the operator wrapping for now, but it seems a bit dodgy.
  2. The ExpressionParser in TokenAnnotator.cpp must not treat this as a binary operator or else it might do weird things with indentation. What't the token type at the moment?
mprobst marked an inline comment as done.Apr 19 2016, 11:32 AM
mprobst added inline comments.
unittests/Format/FormatTestJS.cpp
339 ↗(On Diff #54195)

For (1) if needed we can detect the immediately preceding function.
For (2), token type is tok::star. Indentation in the wrapping example seems fine.

Overall I find it highly unlikely that this becomes a problem, having generator functions that wrap is unlikely, they tend to be top level functions.

Friendly ping.

djasper accepted this revision.Apr 24 2016, 10:07 AM
djasper edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Apr 24 2016, 10:07 AM
This revision was automatically updated to reflect the committed changes.