klimek added inline comments. ================ Comment at: include/clang/Format/Format.h:855 @@ -854,1 +854,3 @@ +// \brief Returns a string representation of ``Language`` for debugging. +inline StringRef getLanguageName(FormatStyle::LanguageKind Language) { ---------------- s/for debugging/. Or do you want to claim that we'll arbitrarily change the strings here?
================ Comment at: lib/Format/Format.cpp:1217-1224 @@ -1983,8 +1216,10 @@ tooling::Replacements sortIncludes(const FormatStyle &Style, StringRef Code, ArrayRef<tooling::Range> Ranges, StringRef FileName, unsigned *Cursor) { tooling::Replacements Replaces; if (!Style.SortIncludes) return Replaces; + if (Style.Language == FormatStyle::LanguageKind::LK_JavaScript) + return sortJavaScriptImports(Style, Code, Ranges, FileName); ---------------- I'd make this symmetric by pulling out 2 functions instead of falling through into the function. ================ Comment at: lib/Format/SortJavaScriptImports.cpp:45 @@ +44,3 @@ + +struct JsImportExport { + bool IsExport; ---------------- klimek wrote: > This is really weird - a class called ImportExport that then has a bool > IsExport. Please explain in a comment why you're doing that instead of any of: > a) managing a set of imports and exports on their own > b) calling this something else > c) having a class hierarchy As not all of the members will be default initialized I'd prefer to have constructors. ================ Comment at: lib/Format/SortJavaScriptImports.cpp:45-46 @@ +44,4 @@ + +struct JsImportExport { + bool IsExport; + // JS imports are sorted into these categories, in order. ---------------- This is really weird - a class called ImportExport that then has a bool IsExport. Please explain in a comment why you're doing that instead of any of: a) managing a set of imports and exports on their own b) calling this something else c) having a class hierarchy ================ Comment at: lib/Format/SortJavaScriptImports.cpp:115 @@ +114,3 @@ + LineEnd = Line->Last; + JsImportExport ImpExp; + skipComments(); ---------------- When I see ImpExp I think "ImplicitExpression". I'd name this ImportExport. Also, it seems like we now have an uninitialized bool and enum in the struct. ================ Comment at: lib/Format/SortJavaScriptImports.cpp:163 @@ +162,3 @@ + return Result; + + // Replace all existing import/export statements. ---------------- If that's the case, please add a comment to that end. ================ Comment at: lib/Format/SortJavaScriptImports.cpp:207 @@ +206,3 @@ + + StringRef getSourceText(SourceLocation Start, SourceLocation End) { + const SourceManager &SM = Env.getSourceManager(); ---------------- Usually we call Lexer::getSourceText for that (don't we have a Lexer?) ================ Comment at: lib/Format/SortJavaScriptImports.cpp:252-253 @@ +251,4 @@ + + bool parseImportExportSpecifier(const AdditionalKeywords &Keywords, + JsImportExport &ImpExp) { + // * as prefix from '...'; ---------------- This function is really long - can we perhaps break out smaller syntactic options. ================ Comment at: lib/Format/SortJavaScriptImports.cpp:258 @@ +257,3 @@ + return false; + if (!Current->is(Keywords.kw_as) || !nextToken()) + return false; ---------------- Please don't put side-effects into an if (). This might be generally easier to read if nextToken() can't fail, and instead creates an invalid token. http://reviews.llvm.org/D20198 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits