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

[Manuel Klimek]

================
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;
----------------
Daniel Jasper wrote:
> Consider undoing this. The patch is already quite complex and we can send a separate formatting cleanup patch afterwards.
Done.

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

================
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,
----------------
Daniel Jasper wrote:
> Same as above.
Done.

================
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);
----------------
Daniel Jasper wrote:
> 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()
This was to check that we don't touch the same token twice. Anyway, it's now changed to output something for every token. Also, I think this will become much nicer once we restructure the token handling.

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

================
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)) {
----------------
Daniel Jasper wrote:
> !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 = ...;
I agree with the inverse logic, but there are too many ways to write down a token check :(

================
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());
----------------
Daniel Jasper wrote:
> Use SourceManager::isBeforeInTranslationUnit()
Done.

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

================
Comment at: lib/Format/WhitespaceManager.h:101-103
@@ +100,5 @@
+
+    bool IsComment;
+    bool IsEOF;
+    bool IsTrailingComment;
+
----------------
Daniel Jasper wrote:
> Can all combinations be set? Otherwise consider using an enum.
Changed to use tok::TokenKind. We don't use this completely consistently yet, due to the BreakableToken still wanting to do its own alignment, but this is the target layout I had in mind.


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