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