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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
