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

[Martin Probst via cfe-commits]

mprobst marked 2 inline comments as done.

================
Comment at: lib/Format/TokenAnnotator.cpp:155
@@ +154,3 @@
+    } else if (Style.Language == FormatStyle::LK_JavaScript && Left->Previous &&
+               Left->Previous->is(TT_JsTypeColon)) {
+      // let x: (SomeType);
----------------
djasper wrote:
> 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)
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.

================
Comment at: lib/Format/TokenAnnotator.cpp:926
@@ +925,3 @@
+        // Type aliases use `type X = ...;` in TypeScript.
+        !(Style.Language == FormatStyle::LK_JavaScript &&
+          Line.First->is(Keywords.kw_type)) &&
----------------
djasper wrote:
> I'd move the ! into the parentheses, but doesn't matter much.
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.


http://reviews.llvm.org/D21597