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

[Daniel Jasper via cfe-commits]

djasper added inline comments.

================
Comment at: lib/Format/Format.cpp:21
@@ -19,1 +20,3 @@
 #include "TokenAnnotator.h"
+#include "FormatTokenLexer.h"
+#include "TokenAnalyzer.h"
----------------
Use clang-format to fix the order :-)

================
Comment at: lib/Format/FormatTokenLexer.h:28
@@ +27,3 @@
+
+class FormatTokenLexer {
+public:
----------------
I believe it is a bit harmful to have that much code in a header. It now gets pulled into multiple translation units and is thus compiled multiple times only for the linker to deduplicate it afterwards. We should move the implementation in FormatTokenLexer.cpp. I am happy to do that as a follow up, though.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:54
@@ +53,3 @@
+  JsImportCategory Category;
+  // Empty for `export {a, b};`.
+  StringRef URL;
----------------
This comment doesn't really explain what it is and it is not obvious to me.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:112
@@ +111,3 @@
+      skipComments();
+      if (LastStart.isInvalid() || Imports.empty()) {
+        // After the first file level comment, consider line comments to be part
----------------
No braces.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:150
@@ +149,3 @@
+
+    bool OutOfOrder = false;
+    for (unsigned i = 0, e = Indices.size(); i != e; ++i) {
----------------
Add:

  // FIXME: Pull this into a common function.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:162
@@ +161,3 @@
+    std::string ImportsText;
+    for (unsigned i = 0, e = Indices.size(); i != e; ++i) {
+      JsImportExport ImpExp = Imports[Indices[i]];
----------------
Is there any chance you are changing the total length of an import block?

================
Comment at: lib/Format/SortJavaScriptImports.cpp:226
@@ +225,3 @@
+      ImpExp.URL = Current->TokenText.substr(1, Current->TokenText.size() - 2);
+      if (ImpExp.URL.startswith("..")) {
+        ImpExp.Category = JsImportExport::JsImportCategory::RELATIVE_PARENT;
----------------
No braces.

================
Comment at: lib/Format/SortJavaScriptImports.cpp:291
@@ +290,3 @@
+                                            unsigned *Cursor) {
+  // TODO(martinprobst): Cursor support.
+  std::unique_ptr<Environment> Env =
----------------
s/TODO(martinprobst)/FIXME/

Also, I'd probably remove the parameter until this is supported. This FIXME is quite hidden but if there is no Cursor in the interface, there is no confusion.

================
Comment at: lib/Format/TokenAnalyzer.h:1
@@ +1,2 @@
+//===--- TokenAnalyzer.h - Analyze Token Streams ----------------*- C++ -*-===//
+//
----------------
This isn't quite as bad as the other, but we probably want a dedicated .cpp file, too.


http://reviews.llvm.org/D20198