djasper added inline comments.
================
Comment at: include/clang/Format/Format.h:770
@@ +769,3 @@
+/// \brief Returns the replacements corresponding to applying, fixing, and
+/// reformatting \p Replaces.
+tooling::Replacements fixReplacements(StringRef Code,
----------------
I think we should not automatically format the replacements after fixing. This
function can easily be combined with formatReplacements if that is desired.
================
Comment at: include/clang/Format/Format.h:802
@@ +801,3 @@
+///
+/// Otherwise identical to the reformat() function using a file ID.
+tooling::Replacements fix(const FormatStyle &Style, StringRef Code,
----------------
This sentence doesn't make sense.
================
Comment at: include/clang/Format/Format.h:803
@@ +802,3 @@
+/// Otherwise identical to the reformat() function using a file ID.
+tooling::Replacements fix(const FormatStyle &Style, StringRef Code,
+ ArrayRef<tooling::Range> Ranges,
----------------
I believe the style will become important as certain aspects of fixing are
going to be controlled via the style. E.g. some styles might want to turn:
if (a)
return;
into:
if (a) {
return;
}
================
Comment at: lib/Format/Format.cpp:1446
@@ -1445,1 +1445,3 @@
+// Returns true if 'Range' intersects with one of the input ranges.
+static bool affectsCharSourceRange(SourceManager &SourceMgr,
----------------
I'd probably move all of these out into a class so you don't need to pass the
same set of parameters between all of them and so that you can make those
private that aren't meant to be called directly.
================
Comment at: lib/Format/Format.cpp:1596
@@ +1595,3 @@
+
+class Fixer : public UnwrappedLineConsumer {
+public:
----------------
I am not sure, this is the right class to pull out. It still has a lot of
overlap with formatter. Maybe it is instead a better idea to pull out each
fixer into its own class and pull them into the formatter.
================
Comment at: lib/Format/Format.cpp:1651
@@ +1650,3 @@
+
+ tooling::Replacements fix(FormatTokenLexer &Tokens, bool *IncompleteFormat) {
+ reset();
----------------
The Tokens parameter is unused!?
================
Comment at: lib/Format/Format.cpp:1654
@@ +1653,3 @@
+
+ while (CurrentToken->isNot(tok::eof)) {
+ switch (CurrentToken->Tok.getKind()) {
----------------
I am not (yet) convinced that the fundamental design here makes sense. So far
it seems to me that all fixers will either operate within a single line (ctor
fixer and probably most others) or that they do something specific on multiple
lines (namespace fixer and empty public/private section fixer and maybe
others). Creating a new way to iterate over all tokens of all lines doesn't
really add useful functionality here AFAICT.
================
Comment at: lib/Format/Format.cpp:1657
@@ +1656,3 @@
+ case tok::colon:
+ if (CurrentToken->Type == TT_CtorInitializerColon) {
+ checkConstructorInitList();
----------------
No braces. Here and at all other single-statement ifs.
================
Comment at: lib/Format/Format.cpp:1690
@@ +1689,3 @@
+
+ void reset() {
+ CurrentLine = 0;
----------------
Having a function called "reset" that is only called once is weird. Just do
this in the constructor?
================
Comment at: lib/Format/Format.cpp:1748
@@ +1747,3 @@
+ unsigned Idx = 0;
+ while (Idx < Tokens.size()) {
+ unsigned St = Idx, Ed = Idx;
----------------
Comment on what this is doing.
================
Comment at: lib/Format/Format.cpp:1749
@@ +1748,3 @@
+ while (Idx < Tokens.size()) {
+ unsigned St = Idx, Ed = Idx;
+ while ((Ed + 1) < Tokens.size() && Tokens[Ed]->Next == Tokens[Ed + 1]) {
----------------
Ed? Either use E or End.
================
Comment at: lib/Format/Format.cpp:1891
@@ +1890,3 @@
+
+ struct FormatTokenLess {
+ FormatTokenLess(SourceManager &SM) : SM(SM) {}
----------------
Why is this needed?
================
Comment at: lib/Format/Format.cpp:1909
@@ +1908,3 @@
+ SmallVector<AnnotatedLine *, 16> AnnotatedLines;
+ size_t CurrentLine;
+ FormatToken DummyToken;
----------------
A lot of these need documentation. What's the current line? What is the last
token (last token that was read? last token of line? last token of file?). What
are RedundantTokens?
================
Comment at: lib/Format/Format.cpp:2356
@@ +2355,3 @@
+
+ std::string NewCode = applyAllReplacements(Code, Replaces);
+ std::vector<tooling::Range> ChangedRanges =
----------------
Too much code duplication. Find a way to avoid that (passing in the calls to
either reformat() or fix() as lambdas to a shared internal function).
================
Comment at: unittests/Format/FormatTest.cpp:11236
@@ -11235,1 +11235,3 @@
+class FixTest : public ::testing::Test {
+protected:
----------------
All of this should go into a different file.
http://reviews.llvm.org/D18551
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
