[PATCH] Format code around VCS conflict markers.

[Manuel Klimek]

  (sending comments I had forgotten to send earlier...)


================
Comment at: lib/Format/Format.cpp:1268
@@ +1267,3 @@
+    FileID ID;
+    unsigned offset;
+    std::tie(ID, offset) = SourceMgr.getDecomposedLoc(
----------------
Daniel Jasper wrote:
> s/offset/Offset/
> 
> Also, offset of what? Consider adding a comment or using a more specific name.
Done.

================
Comment at: lib/Format/Format.cpp:1272
@@ +1271,3 @@
+    StringRef Buffer = SourceMgr.getBuffer(ID)->getBuffer();
+    auto LineOffset = Buffer.rfind('\n', offset);
+    if (LineOffset == StringRef::npos) {
----------------
Daniel Jasper wrote:
> "LineOffset" carries very little meaning, or at least I can't understand what it means without carefully looking at the code. Maybe add a comment.
Done.

================
Comment at: lib/Format/Format.cpp:1285
@@ +1284,3 @@
+    // we expect a space.
+    bool GitMarkerSize = Line.size() <= 7 || Line[7] == ' ' || Line[7] == '\n';
+    // Perforce markers are 4 characters long.
----------------
Daniel Jasper wrote:
> ..Size is not a good name for a bool. Here and below. Just remove "Size"?
Changed to parse out the start of the line until the first whitespace and use that.

================
Comment at: lib/Format/Format.cpp:1307-1309
@@ +1306,5 @@
+      Tokens.resize(FirstInLineIndex + 1);
+      // We do not need to build a complete token here, as we will skip it
+      // during parsing anyway (as we must not touch whitespace around conflict
+      // markers).
+      Tokens.back()->Type = Type;
----------------
Daniel Jasper wrote:
> I think a comment here would help what an expected conflict line together with the generated tokens would look like. Possibly here or at the start of the function.
Done, hopefully...

================
Comment at: lib/Format/Format.cpp:1392
@@ -1391,3 +1454,2 @@
   }
-
   FormatToken *FormatTok;
----------------
Daniel Jasper wrote:
> I liked this empty line.
Done.

================
Comment at: lib/Format/Format.cpp:1467
@@ -1404,1 +1466,3 @@
+  // Index of the last token that starts a new line in 'Tokens'.
+  unsigned FirstInLineIndex;
   SmallVector<FormatToken *, 16> Tokens;
----------------
Daniel Jasper wrote:
> Just to make this even clearer:
> 
>   // Index (in 'Tokens') of the last token that starts a new line.
Done.

================
Comment at: lib/Format/FormatToken.h:67
@@ -66,1 +66,3 @@
   TT_UnaryOperator,
+  TT_ConflictStart,
+  TT_ConflictAlternative,
----------------
Daniel Jasper wrote:
> While this order makes sense, I think we should keep this alphabetical.
Done.

================
Comment at: unittests/Format/FormatTest.cpp:15
@@ -14,2 +14,3 @@
 #include "llvm/Support/Debug.h"
+#include "llvm/ADT/Twine.h"
 #include "gtest/gtest.h"
----------------
Daniel Jasper wrote:
> Remove?
Done.

================
Comment at: lib/Format/UnwrappedLineParser.cpp:468
@@ -467,10 +467,3 @@
 
-void UnwrappedLineParser::pushPPConditional() {
-  if (!PPStack.empty() && PPStack.back() == PP_Unreachable)
-    PPStack.push_back(PP_Unreachable);
-  else
-    PPStack.push_back(PP_Conditional);
-}
-
-void UnwrappedLineParser::parsePPIf(bool IfDef) {
+void UnwrappedLineParser::conditionalCompilationStart(bool Unreachable) {
   ++PPBranchLevel;
----------------
Daniel Jasper wrote:
> I think "Unreachable" needs some explanation...
I think that generally the names are bad. I should rename them, but not in this CL. Added a comment that hopefully helps a bit.

================
Comment at: lib/Format/UnwrappedLineParser.cpp:1418-1431
@@ +1417,16 @@
+           FormatTok->Type == TT_ConflictAlternative) {
+      switch (FormatTok->Type) {
+      case TT_ConflictStart:
+        conditionalCompilationStart(/*Unreachable=*/false);
+        break;
+      case TT_ConflictAlternative:
+        conditionalCompilationAlternative();
+        break;
+      case TT_ConflictEnd:
+        conditionalCompilationEnd();
+        break;
+      default:
+        llvm_unreachable("Unexpected token type.");
+        break;
+      }
+      FormatTok = Tokens->getNextToken();
----------------
Daniel Jasper wrote:
> This would be just 6 lines with if () ..
Done.


http://reviews.llvm.org/D3355

BRANCH
  implement-conflict-markers

ARCANIST PROJECT
  clang