[PATCH] Major refactoring of BreakableToken.
Daniel Jasper
djasper at google.com
Mon May 27 07:13:42 PDT 2013
A couple of nits, but I think this looks very nice!
================
Comment at: lib/Format/BreakableToken.h:31
@@ +30,3 @@
+///
+/// FIXME: The interface seems to be so set in stone that we
+/// might want to just pull the strategy into the class, instead
----------------
s/to be //
(otherwise this sentence does not really parse for me, but might be my bad)
================
Comment at: lib/Format/BreakableToken.h:53
@@ +52,3 @@
+ /// \brief Returns a range (offset, length) at which to break the line at
+ /// \p LineIndex, if previously broken at \p TailOffset if possible not
+ /// violate \p ColumnLimit.
----------------
Sentence does not really parse (2x if)?
================
Comment at: lib/Format/BreakableToken.h:45
@@ +44,3 @@
+ /// \brief Returns the rest of the length of the line at \p LineIndex,
+ /// when broken in \p TailOffset.
+ ///
----------------
s/in/at/?
================
Comment at: lib/Format/BreakableToken.h:58
@@ +57,3 @@
+
+ /// \brief Emits the previuosly retrieved \p Split via the
+ /// \p Whitespacemanager.
----------------
s/uos/ous/
================
Comment at: lib/Format/BreakableToken.h:59
@@ -44,1 +58,3 @@
+ /// \brief Emits the previuosly retrieved \p Split via the
+ /// \p Whitespacemanager.
virtual void insertBreak(unsigned LineIndex, unsigned TailOffset, Split Split,
----------------
I thinks with \p it should be \p Whitespaces.
================
Comment at: lib/Format/BreakableToken.h:114
@@ -209,3 +113,3 @@
public:
- BreakableBlockComment(const SourceManager &SourceMgr,
- const AnnotatedToken &Token, unsigned StartColumn);
+ // \brief Creates a breakable token for a line comment.
+ //
----------------
Combination of non-doxygen comments and doxygen escape-sequences seems weird.
================
Comment at: lib/Format/BreakableToken.cpp:324
@@ +323,3 @@
+ StringRef Prefix = Decoration;
+ if (LineIndex + 1 == Lines.size() && Lines[LineIndex].empty()) {
+ Prefix = "";
----------------
Remove {}.
I saw your discussion on this afterwards. I personally also don't care as much, but we should be consistent. And I think the common LLVM style is to remove this (at least I was asked to do that several times).
================
Comment at: lib/Format/UnwrappedLineParser.h:77
@@ +76,3 @@
+
+ /// \brief The raw text of the token.
+ StringRef TokenText;
----------------
I think this needs some explanation of what it actually contains, especially with respect to escaped newlines. E.g. I think it now does remove all leading escaped new lines ("test" instead of "\\\ntest"), but it still contains escaped newlines everywhere else.
================
Comment at: lib/Format/Format.cpp:833
@@ -831,3 +832,3 @@
unsigned PositionAfterLastLineInToken = 0;
for (unsigned LineIndex = 0; LineIndex < Token->getLineCount();
++LineIndex) {
----------------
I think the convention would be to use:
for (unsigned LineIndex = 0, EndIndex = Token->getLineCount();
LineIndex < EndIndex; ++LineIndex)
I know, this is not changed in this patch, but it is very close and you are actually making it hard to know that "getLineCount()" has constant runtime.
================
Comment at: lib/Format/Format.cpp:817
@@ -817,1 +816,3 @@
+ new BreakableBlockComment(Style, Current.FormatTok, StartColumn,
+ OriginalStartColumn, Current.Parent == 0);
Token.reset(BBC);
----------------
We mostly don't use "== 0" checks. But I don't feel strongly.
================
Comment at: lib/Format/BreakableToken.cpp:203-204
@@ +202,4 @@
+ // and available horizontal space can be too small to align consecutive
+ // lines with the first one. We could, probably, align them to current
+ // indentation level, but now we just wrap them without stars.
+ NeedsStar = false;
----------------
The last sentence sounds like a FIXME ;-)
================
Comment at: lib/Format/BreakableToken.cpp:168
@@ +167,3 @@
+ const char *KnownPrefixes[] = { "/// ", "///", "// ", "//" };
+ for (size_t i = 0; i < llvm::array_lengthof(KnownPrefixes); ++i)
+ if (Comment.startswith(KnownPrefixes[i]))
----------------
Maybe:
for (size_t i = 0, e = llvm::array_lengthof(KnownPrefixes); i < e; ++i)
================
Comment at: lib/Format/BreakableToken.cpp:119
@@ +118,3 @@
+ StringRef::size_type SplitPoint = getStartOfCharacter(Text, MaxSplit);
+ if (SplitPoint != StringRef::npos && SplitPoint > 1)
+ // Do not split at 0.
----------------
Together with the comment, I would invert this condition:
if (SplitPoint == 0)
return BreakableToken::Split(StringRef::npos, 0);
return BreakableToken::Split(SplitPoint, 0);
I think you don't even need the comment then ..
http://llvm-reviews.chandlerc.com/D864
BRANCH
breakable-tokens
ARCANIST PROJECT
clang
More information about the cfe-commits
mailing list