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

[Manuel Klimek via cfe-commits]

klimek added inline comments.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:46-47
@@ +45,4 @@
+// An ES6 module reference.
+//
+// ES6 implements a module system, where individual modules (~= source files)
+// can reference other modules, either importing symbols from them, or exporting
----------------
Thanks, this is much better.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:84-85
@@ +83,4 @@
+  // comments.
+  SourceLocation Start;
+  SourceLocation End;
+};
----------------
Any reason you're not using a SourceRange?

================
Comment at: lib/Format/SortJavaScriptImports.cpp:93-96
@@ +92,6 @@
+    return LHS.Category < RHS.Category;
+  if (LHS.Category == JsModuleReference::ReferenceCategory::SIDE_EFFECT)
+    // Side effect imports might be ordering sensitive. Consider them equal so
+    // that they maintain their relative order in the stable sort below.
+    return false;
+  // Empty URLs sort *last* (for export {...};).
----------------
Doesn't that break strict weak ordering requirements?

x = non-side-effect (url: a)
y = side-effect (url: b)
z = non-side-effect (url: c)
now x and y are incomparable:
!(x < y) // because x.url < y.url
&& !(y < x) // because of y == side-effect
but x and z are comparable:
x < z
breaks transitivity of incomparability (https://en.wikipedia.org/wiki/Weak_ordering#Strict_weak_orderings)

To keep them equal, wouldn't you need:
  if (LHS == SIDE_EFFECT && RHS == SIDE_EFFECT)
    return false;
(that would at least seem symmetric)

================
Comment at: lib/Format/SortJavaScriptImports.cpp:128
@@ +127,3 @@
+
+    SmallVector<JsModuleReference, 16> Imports;
+    SourceLocation LastStart;
----------------
It seems like you can pull the loop below into its own function parseModuleReferences or something.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:129
@@ +128,3 @@
+    SmallVector<JsModuleReference, 16> Imports;
+    SourceLocation LastStart;
+    for (auto Line : AnnotatedLines) {
----------------
LastStart is just the start of the currently processed module reference, right? I'd just call it Start then.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:135
@@ +134,3 @@
+      LineEnd = Line->Last;
+      JsModuleReference Reference;
+      skipComments();
----------------
Why are you defining this here, far before its use?

I think this code might be slightly simpler (for me to understand) if you just pulled Reference out of the loop, and got rid of LastStart.




http://reviews.llvm.org/D20198