[PATCH] Change design for handling escaped newline and trailing comment layout.

[Daniel Jasper]

  Just a few comments for now.

  The main thing I am not yet sure about is the current is the Change struct itself. If I understand correctly, a lot of the functionality relies on us generating at least one change for each token, which is not intuitive and breaking this assumption at some point will have weird consequences. I am not quite sure how to fix this. Maybe we should rename it, but I am not sure that that is enough. Any ideas?


================
Comment at: include/clang/Format/Format.h:140
@@ -139,5 +139,3 @@
            SpacesBeforeTrailingComments == R.SpacesBeforeTrailingComments &&
-           Standard == R.Standard &&
-           IndentWidth == R.IndentWidth &&
-           UseTab == R.UseTab &&
-           BreakBeforeBraces == R.BreakBeforeBraces;
+           Standard == R.Standard && IndentWidth == R.IndentWidth &&
+           UseTab == R.UseTab && BreakBeforeBraces == R.BreakBeforeBraces;
----------------
Consider undoing this. The patch is already quite complex and we can send a separate formatting cleanup patch afterwards.

================
Comment at: lib/Format/BreakableToken.cpp:145
@@ -147,1 +144,3 @@
+
+  // FIXME: This seems wrong...
   if (!Text.endswith(" ") && !InPPDirective)
----------------
Elaborating on your thoughts might help in case you forget .. I don't think it is obvious ..

================
Comment at: lib/Format/BreakableToken.cpp:152
@@ -154,2 +151,3 @@
   unsigned CharsToRemove = Text.size() - TrimmedLine.size() + 1;
+  // FIXME: The 0 is bad.
   Whitespaces.breakToken(Tok, BreakOffset, CharsToRemove, "", "", InPPDirective,
----------------
Same as above.

================
Comment at: lib/Format/Format.cpp:1263
@@ -1281,8 +1262,3 @@
         PreviousLineWasTouched = false;
-        if (TheLine.Last->is(tok::comment))
-          Whitespaces.addUntouchableComment(
-              SourceMgr.getSpellingColumnNumber(
-                  TheLine.Last->FormatTok.Tok.getLocation()) -
-              1);
-        else
-          Whitespaces.alignComments();
+        if (&TheLine.First != TheLine.Last)
+          Whitespaces.addUntouchableToken(*TheLine.Last, TheLine.InPPDirective);
----------------
I don't fully understand why you are changing this (yet), but if you want to check whether the line as exactly one token, please do something like:

TheLine.First.Children.empty()

================
Comment at: lib/Format/TokenAnnotator.cpp:937
@@ +936,3 @@
+    if (!Current->CanBreakBefore &&
+        Current->FormatTok.Tok.isNot(tok::comment) &&
+        Current->FormatTok.Tok.isNot(tok::string_literal)) {
----------------
!Current->FormatTok.Tok.isOneOf(tok::comment, tok::string_literal)


Also, I'd find the inverse logic more intuitive, i.e.:

  if (Current->CanBreakBefore || Current->isOneOf(...))
    UnbreakableSequenceLength = 0;
  else
    UnbreakableSequenceLength = ...;

================
Comment at: lib/Format/TokenAnnotator.h:159
@@ +158,3 @@
+  /// or the next token that can be broken.
+  unsigned UnbreakableSequenceLength;
+
----------------
maybe: UnbreakableTailLength?

================
Comment at: lib/Format/WhitespaceManager.h:101-103
@@ +100,5 @@
+
+    bool IsComment;
+    bool IsEOF;
+    bool IsTrailingComment;
+
----------------
Can all combinations be set? Otherwise consider using an enum.

================
Comment at: lib/Format/WhitespaceManager.h:118
@@ +117,3 @@
+    bool operator()(const Change &C1, const Change &C2) const {
+      return SM.getFileOffset(C1.OriginalWhitespaceRange.getBegin()) <
+             SM.getFileOffset(C2.OriginalWhitespaceRange.getBegin());
----------------
Use SourceManager::isBeforeInTranslationUnit()

================
Comment at: lib/Format/WhitespaceManager.h:126
@@ -104,1 +125,3 @@
 
+  void calculateMissingInformation();
+  /// \brief Try to align all stashed comments.
----------------
This name is too generic. Either find a better one or at least add a comment.


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