[PATCH] Implement correct indentation across preprocessor branches.

Manuel Klimek klimek at google.com
Fri Oct 11 13:06:06 PDT 2013



================
Comment at: lib/Format/Format.cpp:1252
@@ +1251,3 @@
+    assert(!UnwrappedLines.empty());
+    UnwrappedLines.rbegin()->push_back(TheLine);
+  }
----------------
Daniel Jasper wrote:
> I would use back(), but probably irrelevant ..
Done.

================
Comment at: lib/Format/FormatToken.h:78
@@ -77,1 +77,3 @@
 
+enum FormatState {
+  FS_Unformatted,
----------------
Daniel Jasper wrote:
> I don't like this name as it is too closely related to LineState and ParenState. We'll eventually confuse LineState (which actually is a FormatState with this). Maybe FormatDecision?
Done.

================
Comment at: lib/Format/FormatToken.h:350
@@ +349,3 @@
+
+  /// \brief If true, this token has been fully formatted (indented and
+  /// potentially re-formatted inside), and we do not allow further formatting
----------------
Daniel Jasper wrote:
> nit: If \c true, ...
Done.

================
Comment at: lib/Format/FormatToken.h:347
@@ -339,1 +346,3 @@
 
+  /// \brief Stores the formatting decision for the token once it was made.
+  FormatState State;
----------------
Daniel Jasper wrote:
> Ha, so you agree that it is a decision (didn't see this before making the name suggestion above) :-)
Ack.

================
Comment at: lib/Format/UnwrappedLineParser.cpp:194
@@ +193,3 @@
+void UnwrappedLineParser::reset() {
+  PPBranchLevel = -1;
+  Line.reset(new UnwrappedLine);
----------------
Daniel Jasper wrote:
> Seems weird to initialize this to 0 in the constructor and to -1 here
Done.

================
Comment at: lib/Format/UnwrappedLineParser.cpp:223
@@ +222,3 @@
+         I != E; ++I) {
+      Callback.consumeUnwrappedLine(*I);
+    }
----------------
Daniel Jasper wrote:
> This seems like a really weird interface now (iterating over an array, adding each line and then calling finish). Would it make sense to change to a single method call where we can pass in an array? Then on the other side you could save the logic to add an empty array for the next set of lines in finishRun(), which is not really intuitive..
I agree, but I'd like to change that in a follow-up patch. I think we might just want to return the unwrapped lines here, as they'll now get copied anyway.

================
Comment at: lib/Format/UnwrappedLineParser.cpp:228
@@ +227,3 @@
+    while (!PPLevelBranchIndex.empty() &&
+           *PPLevelBranchIndex.rbegin() + 1 == *PPLevelBranchCount.rbegin()) {
+      PPLevelBranchIndex.resize(PPLevelBranchIndex.size() - 1);
----------------
Daniel Jasper wrote:
> Use PPLevelBranchIndex.back() here and below.
Done.


http://llvm-reviews.chandlerc.com/D1883



More information about the cfe-commits mailing list