[PATCH] Change design for handling escaped newline and trailing comment layout.
Daniel Jasper
djasper at google.com
Tue May 21 22:43:32 PDT 2013
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
More information about the cfe-commits
mailing list