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

[Manuel Klimek via cfe-commits]

klimek added inline comments.

================
Comment at: include/clang/Format/Format.h:855
@@ -854,1 +854,3 @@
 
+// \brief Returns a string representation of ``Language`` for debugging.
+inline StringRef getLanguageName(FormatStyle::LanguageKind Language) {
----------------
s/for debugging/.
Or do you want to claim that we'll arbitrarily change the strings here?

================
Comment at: lib/Format/Format.cpp:1217-1224
@@ -1983,8 +1216,10 @@
 
 tooling::Replacements sortIncludes(const FormatStyle &Style, StringRef Code,
                                    ArrayRef<tooling::Range> Ranges,
                                    StringRef FileName, unsigned *Cursor) {
   tooling::Replacements Replaces;
   if (!Style.SortIncludes)
     return Replaces;
+  if (Style.Language == FormatStyle::LanguageKind::LK_JavaScript)
+    return sortJavaScriptImports(Style, Code, Ranges, FileName);
 
----------------
I'd make this symmetric by pulling out 2 functions instead of falling through into the function.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:45
@@ +44,3 @@
+
+struct JsImportExport {
+  bool IsExport;
----------------
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.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:45-46
@@ +44,4 @@
+
+struct JsImportExport {
+  bool IsExport;
+  // JS imports are sorted into these categories, in order.
----------------
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

================
Comment at: lib/Format/SortJavaScriptImports.cpp:115
@@ +114,3 @@
+      LineEnd = Line->Last;
+      JsImportExport ImpExp;
+      skipComments();
----------------
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.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:163
@@ +162,3 @@
+      return Result;
+
+    // Replace all existing import/export statements.
----------------
If that's the case, please add a comment to that end.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:207
@@ +206,3 @@
+
+  StringRef getSourceText(SourceLocation Start, SourceLocation End) {
+    const SourceManager &SM = Env.getSourceManager();
----------------
Usually we call Lexer::getSourceText for that (don't we have a Lexer?)

================
Comment at: lib/Format/SortJavaScriptImports.cpp:252-253
@@ +251,4 @@
+
+  bool parseImportExportSpecifier(const AdditionalKeywords &Keywords,
+                                  JsImportExport &ImpExp) {
+    // * as prefix from '...';
----------------
This function is really long - can we perhaps break out smaller syntactic options.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:258
@@ +257,3 @@
+        return false;
+      if (!Current->is(Keywords.kw_as) || !nextToken())
+        return false;
----------------
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.


http://reviews.llvm.org/D20198