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

[Martin Probst via cfe-commits]

mprobst marked 6 inline comments as done.
mprobst added a comment.

PTAL.


================
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 {...};).
----------------
klimek wrote:
> 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)
As discussed offline, the comparison before makes sure that RHS == SIDE_EFFECT here.

Added a comment.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:135
@@ +134,3 @@
+      LineEnd = Line->Last;
+      JsModuleReference Reference;
+      skipComments();
----------------
klimek wrote:
> 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.
> 
> 
As discussed offline, moved the declaration a bit down.

I think exposing just a `SourceLocation Start` makes the algorithm easier to read, otherwise it'd look like a whole `JsModuleReference` could leave the loop, which is never the case. It explicitly points out we only have state outside of the loop to track the start.


http://reviews.llvm.org/D20198