[PATCH] D20198: clang-format: [JS] sort ES6 imports.

[Martin Probst via cfe-commits]

mprobst added a comment.

PTAL.


================
Comment at: lib/Format/SortJavaScriptImports.cpp:45-46
@@ +44,4 @@
+
+struct JsImportExport {
+  bool IsExport;
+  // JS imports are sorted into these categories, in order.
----------------
klimek wrote:
> klimek wrote:
> > This is really weird - a class called ImportExport that then has a bool IsExport. Please explain in a comment why you're doing that instead of any of:
> > a) managing a set of imports and exports on their own
> > b) calling this something else
> > c) having a class hierarchy
> As not all of the members will be default initialized I'd prefer to have constructors.
Discussed offline. I stand by this code. Yes, it has an odd boolean flag, but imports and exports in ES6 are essentially the same syntactical construct, with exports being syntactic sugar for import followed by export.

I've updated the comment on this struct substantially to explain that (I can tell how it's non-obvious).

Note that the boolean is never dispatched on in this code, except for the "empty line before exports" part. Exports are the same as imports wrt sorting them here.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:115
@@ +114,3 @@
+      LineEnd = Line->Last;
+      JsImportExport ImpExp;
+      skipComments();
----------------
klimek wrote:
> When I see ImpExp I think "ImplicitExpression". I'd name this ImportExport.
> Also, it seems like we now have an uninitialized bool and enum in the struct.
Now called `Reference`.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:207
@@ +206,3 @@
+
+  StringRef getSourceText(SourceLocation Start, SourceLocation End) {
+    const SourceManager &SM = Env.getSourceManager();
----------------
klimek wrote:
> Usually we call Lexer::getSourceText for that (don't we have a Lexer?)
We don't, FormatTokenLexer doesn't expose the `Lexer`, and `TokenAnalyzer` doesn't export `FormatTokenLexer`.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:258
@@ +257,3 @@
+        return false;
+      if (!Current->is(Keywords.kw_as) || !nextToken())
+        return false;
----------------
klimek wrote:
> Please don't put side-effects into an if (). This might be generally easier to read if nextToken() can't fail, and instead creates an invalid token.
Done, much cleaner indeed. LLVM style guide says not to introduce new static globals with constructors, so I'm just using a field. Please advise if there's a better way.


http://reviews.llvm.org/D20198