djasper added inline comments.
================
Comment at: lib/Format/Format.cpp:1654
@@ +1653,3 @@
+ UnwrappedLines.push_back(SmallVector<UnwrappedLine, 16>());
+ }
+
----------------
That I don't understand. Almost all fixers (cleaning up commas, constructor
initializers, etc.) will only ever look at a single line.
The matchers that do make multiline fixes (empty namespaces, empty
public/private sections) only look at complete lines, i.e. only ever use
AnnotatedLine::startsWith(...) and don't iterate over the tokens, AFAICT.
I really think that giving the option to iterate over all tokens of multiple
lines does more harm than good. Among several other things, error recovery is
made harder. It never makes sense for the ctor-initializer fixer to leave its
line (or else you'll just ignore some nice error recovery already implemented
in the UnwrappedLineParser).
================
Comment at: lib/Format/Format.cpp:1693
@@ +1692,3 @@
+ // FIXME: we need better way to determine wether to delete this token.
+ if (!AnnotatedLines[CurrentLine]->Affected && !Forced) return;
+ Tok->Deleted = true;
----------------
I have a comment here (and in some other places), but I think this will be
different anyway if we move to line-based fixers. So I am holding off with
those comments for now.
================
Comment at: lib/Format/Format.cpp:1718
@@ +1717,3 @@
+ void skipParens() {
+ assert(CurrentToken->is(tok::l_paren));
+ nextToken();
----------------
Just go to CurrentToken->MatchingParen!?
================
Comment at: lib/Format/Format.cpp:1824
@@ +1823,3 @@
+ // Returns true if the namespace is empty .
+ bool checkEmptyNamespace() {
+ assert(CurrentToken->is(tok::kw_namespace));
----------------
I am happy to show you how the implementation here gets much simpler if you
only iterate over AnnotatedLines.
================
Comment at: lib/Format/Format.cpp:1916
@@ +1915,3 @@
+ FormatToken *CurrentToken;
+ // Redundant tokens to be deleted.
+ std::set<FormatToken *, FormatTokenLess> RedundantTokens;
----------------
I think "redundant" is the wrong word here. How about DeletedTokens?
http://reviews.llvm.org/D18551
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
