This is an archive of the discontinued LLVM Phabricator instance.

clang-format: [JS] post-fix non-null assertion operator.
ClosedPublic

Authored by mprobst on Jun 9 2016, 2:46 PM.

Details

Summary

Do not insert whitespace preceding the "!" postfix operator. This is an
incomplete fix, but should cover common usage.

Diff Detail

Repository
rL LLVM

Event Timeline

mprobst updated this revision to Diff 60242.Jun 9 2016, 2:46 PM
mprobst retitled this revision from to clang-format: [JS] post-fix non-null assertion operator..
mprobst updated this object.
mprobst added a reviewer: djasper.
mprobst added a subscriber: cfe-commits.
djasper added inline comments.Jun 9 2016, 9:53 PM
lib/Format/TokenAnnotator.cpp
2127 ↗(On Diff #60242)

From a quick look at our codebase, I think you need to also rule out the following for Right.Next:

  • (, [, {
  • ! ('!!' seems to be a thing)
  • -, +, ~ (don't know exactly what this does to a JS boolean value)
  • %, $ (not entirely sure whether these become part of the identifier)
mprobst updated this revision to Diff 60308.Jun 9 2016, 10:08 PM
mprobst marked an inline comment as done.
  • invert check to whitelist known tokens that can precede a non-null assertion.
lib/Format/TokenAnnotator.cpp
2127 ↗(On Diff #60242)

Actually, I think inverting the condition and "whitelisting" preceding tokens makes more sense and is probably a lot more robust. PTAL, now checking left for identifier, r_paren, r_square, literal.

djasper accepted this revision.Jun 9 2016, 10:14 PM
djasper edited edge metadata.
djasper added inline comments.
lib/Format/TokenAnnotator.cpp
2128 ↗(On Diff #60308)

Can you add a test for each of these? Also, how can a literal be null?

This revision is now accepted and ready to land.Jun 9 2016, 10:14 PM
mprobst updated this revision to Diff 60311.Jun 9 2016, 10:22 PM
mprobst edited edge metadata.
  • more tests
lib/Format/TokenAnnotator.cpp
2128 ↗(On Diff #60311)

Tests added. A literal cannot really be null, but the operator is allowed after any kind of expression, and it's straight forward to test for it here.

This revision was automatically updated to reflect the committed changes.