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