mprobst added a comment.
PTAL.
================
Comment at: lib/Format/SortJavaScriptImports.cpp:45-46
@@ +44,4 @@
+
+struct JsImportExport {
+ bool IsExport;
+ // JS imports are sorted into these categories, in order.
----------------
klimek wrote:
> 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.
Discussed offline. I stand by this code. Yes, it has an odd boolean flag, but
imports and exports in ES6 are essentially the same syntactical construct, with
exports being syntactic sugar for import followed by export.
I've updated the comment on this struct substantially to explain that (I can
tell how it's non-obvious).
Note that the boolean is never dispatched on in this code, except for the
"empty line before exports" part. Exports are the same as imports wrt sorting
them here.
================
Comment at: lib/Format/SortJavaScriptImports.cpp:115
@@ +114,3 @@
+ LineEnd = Line->Last;
+ JsImportExport ImpExp;
+ skipComments();
----------------
klimek wrote:
> 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.
Now called `Reference`.
================
Comment at: lib/Format/SortJavaScriptImports.cpp:207
@@ +206,3 @@
+
+ StringRef getSourceText(SourceLocation Start, SourceLocation End) {
+ const SourceManager &SM = Env.getSourceManager();
----------------
klimek wrote:
> Usually we call Lexer::getSourceText for that (don't we have a Lexer?)
We don't, FormatTokenLexer doesn't expose the `Lexer`, and `TokenAnalyzer`
doesn't export `FormatTokenLexer`.
================
Comment at: lib/Format/SortJavaScriptImports.cpp:258
@@ +257,3 @@
+ return false;
+ if (!Current->is(Keywords.kw_as) || !nextToken())
+ return false;
----------------
klimek wrote:
> 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.
Done, much cleaner indeed. LLVM style guide says not to introduce new static
globals with constructors, so I'm just using a field. Please advise if there's
a better way.
http://reviews.llvm.org/D20198
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
