Page MenuHomePhabricator

clang-format: [JS] Handle certain cases of ASI.
ClosedPublic

Authored by mprobst on Mar 5 2016, 12:57 PM.

Details

Reviewers
djasper
Summary

Automatic Semicolon Insertion can only be properly handled by parsing
source code. However conservatively catching just a few, common
situations prevents breaking code during development, which greatly
improves usability.

JS code should still use semicolons, and ASI code should be flagged by
a compiler or linter.

Diff Detail

Event Timeline

mprobst updated this revision to Diff 49882.Mar 5 2016, 12:57 PM
mprobst retitled this revision from to clang-format: [JS] Handle certain cases of ASI..
mprobst updated this object.
mprobst added a reviewer: djasper.
mprobst added subscribers: cfe-commits, klimek.
mprobst updated this revision to Diff 49883.Mar 5 2016, 1:00 PM
  • Handle unary !.
djasper added inline comments.Mar 6 2016, 8:30 AM
lib/Format/UnwrappedLineParser.cpp
667

Make the parameters const.

I'd also like this to have "JS" in the name somehow. And I am worried that this might be confusing because it says "shouldInsert", but we don't insert (at least not yet). Maybe rename this to wouldTriggerJavaScriptASI(...).

670

I think it is a bit confusing to hand in both Previous and Next but still rely on some state of the class. Maybe it is better to move the 4 lines from nextToken into this function?

692

I'd just hand in "Keywords" for now and make this a local (static) function. We should also refactor this whole thing to not have the class definition in the header probably.

Also, this is incorrect as it doesn't return true for C++ keywords that aren't JS keywords, e.g. try "struct". Leave a FIXME for me to sort this out.

1968

const?

1970

In LLVM-style, we generally don't use braces for single-statement ifs.

unittests/Format/FormatTestJS.cpp
64

I don't think it is useful to do this as there are always ways in which formatting can fail leaving the input untouched. We still want to verify that formatting takes place.

To do so, manually mess up the code in some way and use EXPECT_EQ. There should be various examples in FormatTest.cpp.

mprobst updated this revision to Diff 49922.Mar 6 2016, 11:47 AM
mprobst marked 6 inline comments as done.
  • Handle unary !.
  • Address review comments.
unittests/Format/FormatTestJS.cpp
64

Done - introduced a verifyFormat(str, str, style) to make it a less verbose.

mprobst updated this revision to Diff 50537.Mar 12 2016, 6:05 PM
  • Handle unary !.
  • Address review comments.
  • Support declarations and statements in ASI.
djasper edited edge metadata.Mar 14 2016, 12:12 PM

Looks good.

lib/Format/UnwrappedLineParser.cpp
667

We don't usually put usernames into FIXMEs

djasper accepted this revision.Mar 14 2016, 12:26 PM
djasper edited edge metadata.

Submitted as r263470.

This revision is now accepted and ready to land.Mar 14 2016, 12:26 PM
djasper closed this revision.Mar 14 2016, 12:26 PM