Page MenuHomePhabricator

clang-format: [JS] recognize more type locations.
ClosedPublic

Authored by mprobst on Jun 21 2016, 10:09 PM.

Diff Detail

Event Timeline

mprobst updated this revision to Diff 61503.Jun 21 2016, 10:09 PM
mprobst retitled this revision from to clang-format: [JS] recognize more type locations..
mprobst updated this object.
mprobst added a reviewer: djasper.
mprobst added a subscriber: cfe-commits.

This seems to be getting a bit tricky with the declaration contexts. If you have a better suggestion, please come forward.

djasper added inline comments.Jun 22 2016, 12:38 AM
lib/Format/TokenAnnotator.cpp
480

Why is this necessary?

mprobst updated this revision to Diff 61548.Jun 22 2016, 7:41 AM
  • Remove parseJSTypeDefinition
lib/Format/TokenAnnotator.cpp
480

It is not, actually. I was chasing a wrong path here before understanding it was because of the assignment. Removed.

djasper accepted this revision.Jun 23 2016, 3:14 AM
djasper edited edge metadata.

Basically looks good.

lib/Format/TokenAnnotator.cpp
155

I'd merge this one in with the previous.. And maybe even the one from above so that we end up with:

} else if (Style.Language == FormatStyle::LK_JavaScript && Left->Previous &&
           (Line.First->is(Keywords.kw_type) ||
            Left->Previous->isOneOf(Keywords.kw_function, TT_JsTypeColon) ||
            (Left->Previous->endsSequence(tok::identifier,
                                          Keywords.kw_function)))) {

(in order for Line.First to be "type", Left->Previous cannot be nullptr, so this should be equivalent)

664

nit: undo

926

I'd move the ! into the parentheses, but doesn't matter much.

This revision is now accepted and ready to land.Jun 23 2016, 3:14 AM
mprobst marked 2 inline comments as done.Jun 23 2016, 11:49 AM
mprobst added inline comments.
lib/Format/TokenAnnotator.cpp
155

I think I'd prefer keeping this as is. The way it is now formulated is more verbose, but that's to communicate and keep apart three very different situations (type alias, (async)? function, variable declaration).

Merging the boolean conditions in some clever way will make it very hard to reverse-decipher which partially overlapping boolean clause is for which of these situations, and it's usually non-obvious if test coverage is good enough to make sure nothing breaks if you refactor.

The could would also be harder to read, e.g. the is(kw_function) part is logically related to the endsSeqeunce call, but they end up in different parts of the clause.

So, in the interest of readability and maintainability, I'll keep it like this, and assume by accepting the diff you're cool with that :-) If not, please do push back.

926

I've formulated it like this intentionally – it follows my brain waves, as in "mark this as an expression if not this is javascript and it's a type def". Changing it to "mark this as an expression if language is not javascript or line is not a typedef" is less readable to me.

mprobst updated this revision to Diff 61706.Jun 23 2016, 11:51 AM
mprobst marked an inline comment as done.
mprobst edited edge metadata.
  • progress
This revision was automatically updated to reflect the committed changes.