djasper added inline comments.
================
Comment at: include/clang/Format/Format.h:769
@@ -768,1 +768,3 @@
+/// \brief Returns the replacements corresponding to applying and fixing
+/// \p Replaces.
----------------
I am actually not sure that fixReplacements is the right terminology here
(sorry that I haven't discovered this earlier). It isn't really fixing the
replacements (they aren't necessarily broken). How about
cleanupAroundReplacements? Manuel, what do you think?
================
Comment at: lib/Format/Format.cpp:1596
@@ +1595,3 @@
+ RangeMgr(SourceMgr,
+ SmallVector<CharSourceRange, 8>(Ranges.begin(),
Ranges.end())),
+ UnwrappedLines(1),
----------------
But like this, it is way to much code duplication for me, both in number of
lines of code as well as code complexity. Basically everything in fix() is
duplicated along with most of the class members etc. The main difference seems
to be that runFix() is called instead of format. There must be a way to have
only one implementation of that code.
I think (but cannot see all the consequences yet) you should merge the two
classes. And if that leads to too much complexity, you can probably pull out
runFix and everything it calls into a separate class as well as possibly
format() and everything that format calls.
================
Comment at: lib/Format/Format.cpp:1597
@@ +1596,3 @@
+ SmallVector<CharSourceRange, 8>(Ranges.begin(),
Ranges.end())),
+ UnwrappedLines(1),
+ Encoding(encoding::detectEncoding(SourceMgr.getBufferData(ID))),
----------------
Why are you doing this?
================
Comment at: lib/Format/Format.cpp:1664
@@ +1663,3 @@
+
+ if (Line.IsConstructorInitializer)
+ checkConstructorInitList(Line);
----------------
Can you move the ConstructorInitializer fixer and everything that belongs to it
to a different patch? I want to look at that in more detail and two smaller
patches make this easier for me.
================
Comment at: lib/Format/Format.cpp:1672
@@ +1671,3 @@
+ bool containsOnlyComments(const AnnotatedLine &Line) {
+ bool CommentsOnly = true;
+ FormatToken *Tok = Line.First;
----------------
No need for this boolean. Write this as:
for (FormatToken *Tok = Line.First;; Tok = Tok->Next) {
if (!Tok->is(tok::comment))
return false;
}
return true;
And maybe we should (in a separate patch) add an iterator interface to
AnnotatedLine so that we can use range-based for loops.
================
Comment at: lib/Format/Format.cpp:1695
@@ +1694,3 @@
+ if (NewLine == -1) {
+ IsEmpty = false;
+ continue;
----------------
Why not return -1 here?
================
Comment at: lib/Format/Format.cpp:1699
@@ +1698,3 @@
+ CurrentLine = NewLine;
+ } else if (AnnotatedLines[CurrentLine]->startsWith(tok::comment) &&
+ containsOnlyComments(*AnnotatedLines[CurrentLine]))
----------------
Move the startsWith into containsOnlyComments.
================
Comment at: lib/Format/Format.cpp:1701
@@ +1700,3 @@
+ containsOnlyComments(*AnnotatedLines[CurrentLine]))
+ continue;
+ else if (AnnotatedLines[CurrentLine]->startsWith(tok::r_brace))
----------------
I'd write this in a very different order:
if (AnnotatedLines[CurrentLine]->startsWith(tok::r_brace))
break;
if (AnnotatedLines[CurrentLine]->startsWith(kw_namespaces) || ..) {
int NewLine = checkEmptyNamespace(CurrentLine);
if (NewLine == -1)
return -1;
continue;
if (!containsOnlyComments(..))
return -1;
================
Comment at: lib/Format/Format.cpp:1702
@@ +1701,3 @@
+ continue;
+ else if (AnnotatedLines[CurrentLine]->startsWith(tok::r_brace))
+ break;
----------------
It's a bit subtle that this works correctly. At least I needed to think quickly
why you wouldn't falsely run into this in
namespace N {
void f() {
}
}
But of course, then the namespace wouldn't be empty. Please leave a comment
explaining this briefly.
================
Comment at: lib/Format/Format.cpp:1717
@@ +1716,3 @@
+
+ for (unsigned i = InitLine; i <= CurrentLine; ++i) {
+ AnnotatedLines[i]->Deleted = true;
----------------
Hm, this seems very inelegant. If you have:
namespace A {
namespace B {
namespace C {
}
}
}
"namespace C {" and "}" gets delete three times. At the very least you should
add
if (AnnotatedLines[i]->Deleted)
continue;
Thinking about this some more, it will get deleted even more often has even the
outer loop in runFix will call checkEmptyNamespace again for the inner
namespaces even though they have been deleted already. So inner namespaces will
bei deleted O(N^2) times if N is the nesting depth. Lets get that down to 1
instead ;-)
================
Comment at: lib/Format/Format.cpp:1818
@@ +1817,3 @@
+
+ // Merge multiple continuous token deletions into one big deletion.
+ unsigned Idx = 0;
----------------
Can you explain why you are doing this?
================
Comment at: lib/Format/Format.cpp:2286
@@ +2285,3 @@
+ const FormatStyle &, StringRef, std::vector<tooling::Range>, StringRef)>
+ ReplacementsProcessFunctionType;
+
----------------
Don't define a typedef for something that is only used once. Also, this is an
internal function, how about writing this as:
template <typename T>
static tooling::Replacements
processReplacements(StringRef Code, const tooling::Replacements &Replaces, T
ProcessFunc,
const FormatStyle &Style) {
}
No need to spell out the function type (I think).
http://reviews.llvm.org/D18551
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
