djasper added inline comments.
================
Comment at: lib/Format/Format.cpp:1677
@@ +1676,3 @@
+
+class Formatter {
+public:
----------------
I think FormatStyle in the Processor should be const and Formatter should make
its own copy to modify it.
================
Comment at: lib/Format/Format.cpp:1926
@@ +1925,3 @@
+ // Returns true if the current namespace is empty.
+ bool checkEmptyNamespace(unsigned CurrentLine, unsigned &NewLine) {
+ if (!AnnotatedLines[CurrentLine]->Last->is(tok::l_brace)) {
----------------
Why are you introducing a return value that you aren't using? I'd just return
the number of subsequent lines to skip.
Overall, I'd do this a bit differently:
- Let it check the namespace starting at CurrentLine
- Delete empty namespaces within
- If it is empty, delete the outer namespace
- Always return how many lines it has looked at
That way, you don't need to store DeletedLines and you don't ever look at the
same line multiple times.
================
Comment at: lib/Format/Format.cpp:1927
@@ +1926,3 @@
+ bool checkEmptyNamespace(unsigned CurrentLine, unsigned &NewLine) {
+ if (!AnnotatedLines[CurrentLine]->Last->is(tok::l_brace)) {
+ NewLine = CurrentLine;
----------------
What about
namespace A { // really cool namespace
?
================
Comment at: lib/Format/Format.cpp:1987
@@ +1986,3 @@
+ for (unsigned Idx = 0, End = AnnotatedLines.size() - 1; Idx < End; ++Idx) {
+ AnnotatedLines[Idx]->Last->Next = AnnotatedLines[Idx + 1]->First;
+ }
----------------
I believe that this doesn't work in many circumstances. Not sure I can easily
construct a test case for the namespace cleanup, but generally, AnnotatedLines
are not in a strict order. The last token of a line isn't necessarily next to
the first token of the next line. This happens, e.g. for nested blocks and if
preprocessor lines are intermingled with other lines.
================
Comment at: lib/Format/Format.cpp:1990-1993
@@ +1989,6 @@
+
+ // Merge multiple continuous token deletions into one big deletion so that
+ // the number of replacements can be reduced. When we do `reformat` on the
+ // fixed code, there could be fewer changed code ranges, which makes
+ // computing affected lines more efficient.
+ unsigned Idx = 0;
----------------
I think this should hardly matter considering that we are probably only going
to have a few instances of cleanup replacements and the range calculation is
not that inefficient.
================
Comment at: lib/Format/Format.cpp:2224
@@ +2223,3 @@
+ const FormatStyle &Style) {
+ // We need to use lambda function here since `reformat` has default parameter
+ // `IncompleteFormat`.
----------------
Do we also need that if we spell out the ProcessFunc above? Maybe that's
actually less painful compared to writing these lambdas?
http://reviews.llvm.org/D18551
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
