This revision was automatically updated to reflect the committed changes.
Closed by commit rL293055: [clang-format] Implement comment reflowing.
(authored by krasimir).
Changed prior to commit:
https://reviews.llvm.org/D28764?vs=85739=85740#toc
Repository:
rL LLVM
klimek accepted this revision.
klimek added a comment.
This revision is now accepted and ready to land.
LG
https://reviews.llvm.org/D28764
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
krasimir updated this revision to Diff 85739.
krasimir marked 2 inline comments as done.
krasimir added a comment.
- Address review comments.
https://reviews.llvm.org/D28764
Files:
lib/Format/BreakableToken.cpp
lib/Format/BreakableToken.h
lib/Format/CMakeLists.txt
krasimir added inline comments.
Comment at: lib/Format/BreakableToken.cpp:747
+Split SplitBefore, WhitespaceManager ) {
+ // If this is the first line of a token, we need to inform Whitespace Manager
+ // about it: either adapt the whitespace range preceding it, or mark it
klimek added inline comments.
Comment at: lib/Format/BreakableToken.cpp:747
+Split SplitBefore, WhitespaceManager ) {
+ // If this is the first line of a token, we need to inform Whitespace Manager
+ // about it: either adapt the whitespace range preceding it, or mark it
krasimir added a comment.
Tried to add some comments around range computations. Most of the time it's
about converting range offsets from local line-based values to
start-of-token-based offsets.
Comment at: lib/Format/BreakableToken.cpp:279-280
+ return Content.size() >= 2
krasimir updated this revision to Diff 85737.
krasimir marked 3 inline comments as done.
krasimir added a comment.
- Address comments. Add a bunch of comments about various range computations.
https://reviews.llvm.org/D28764
Files:
lib/Format/BreakableToken.cpp
lib/Format/BreakableToken.h
klimek added inline comments.
Comment at: lib/Format/BreakableToken.cpp:279-280
+ return Content.size() >= 2 &&
+ Content != "clang-format on" &&
+ Content != "clang-format off" &&
+ !Content.endswith("\\") &&
Can we now use delimitsRegion here?
krasimir added inline comments.
Comment at: lib/Format/ContinuationIndenter.cpp:1158-1159
+CommentPragmasRegex.match(Current.TokenText.substr(2)) ||
+Current.TokenText.substr(2).ltrim().startswith("clang-format on") ||
+
krasimir updated this revision to Diff 85725.
krasimir marked 8 inline comments as done.
krasimir added a comment.
- Address review comments.
https://reviews.llvm.org/D28764
Files:
lib/Format/BreakableToken.cpp
lib/Format/BreakableToken.h
lib/Format/CMakeLists.txt
klimek added inline comments.
Comment at: lib/Format/BreakableToken.h:42-48
+/// There is a pair of operations that are used to compress a long whitespace
+/// range with a single space if that will bring the line lenght under the
+/// column limit:
+/// -
krasimir marked an inline comment as done.
krasimir added inline comments.
Comment at: lib/Format/BreakableToken.h:42-48
+/// There is a pair of operations that are used to compress a long whitespace
+/// range with a single space if that will bring the line lenght under the
klimek added a comment.
This is starting to be pretty awesome. The one thing that would help me review
the gist of the implementation a bit more is if that had a bit more comments.
Perhaps go over the math in the code and put some comments in why we're doing
what at various steps.
krasimir added inline comments.
Comment at: lib/Format/ContinuationIndenter.cpp:1158-1159
+CommentPragmasRegex.match(Current.TokenText.substr(2)) ||
+Current.TokenText.substr(2).ltrim().startswith("clang-format on") ||
+
klimek added inline comments.
Comment at: lib/Format/ContinuationIndenter.cpp:1158-1159
+CommentPragmasRegex.match(Current.TokenText.substr(2)) ||
+Current.TokenText.substr(2).ltrim().startswith("clang-format on") ||
+
krasimir updated this revision to Diff 85557.
krasimir marked 3 inline comments as done.
krasimir added a comment.
- Address review comments.
https://reviews.llvm.org/D28764
Files:
lib/Format/BreakableToken.cpp
lib/Format/BreakableToken.h
lib/Format/CMakeLists.txt
krasimir marked 3 inline comments as done.
krasimir added inline comments.
Comment at: lib/Format/BreakableToken.h:55-56
+/// been reformatted, and
+/// - replaceWhitespaceBefore, for executing the reflow using a whitespace
+/// manager.
+///
klimek wrote:
>
klimek added inline comments.
Comment at: lib/Format/BreakableToken.h:55-56
+/// been reformatted, and
+/// - replaceWhitespaceBefore, for executing the reflow using a whitespace
+/// manager.
+///
Shouldn't that be called insertBreakBefore for consistency
krasimir updated this revision to Diff 85381.
krasimir added a comment.
- Add back a test case that I had previously removed for no good reason.
https://reviews.llvm.org/D28764
Files:
lib/Format/BreakableToken.cpp
lib/Format/BreakableToken.h
lib/Format/CMakeLists.txt
krasimir updated this revision to Diff 85376.
krasimir added a comment.
- Add a note about replaceWhitespace in the comments of BreakableToken.
https://reviews.llvm.org/D28764
Files:
lib/Format/BreakableToken.cpp
lib/Format/BreakableToken.h
lib/Format/CMakeLists.txt
klimek added inline comments.
Comment at: lib/Format/BreakableToken.h:40
+/// of the content after a split has been used for breaking, and
+/// - insertBreak, for executing the split using a whitespace manager.
+///
Do we want to describe how replaceWhitespace
krasimir added inline comments.
Comment at: lib/Format/BreakableToken.h:87
/// space.
virtual void replaceWhitespace(unsigned LineIndex, unsigned TailOffset,
Split Split,
By the way, I got confused, this stays because the
krasimir updated this revision to Diff 85339.
krasimir added a comment.
- [clang-format] Improve the interface of BreakableToken and add comments.
https://reviews.llvm.org/D28764
Files:
lib/Format/BreakableToken.cpp
lib/Format/BreakableToken.h
lib/Format/CMakeLists.txt
krasimir created this revision.
krasimir added a reviewer: klimek.
krasimir added subscribers: ioeric, cfe-commits, mgorny, klimek, djasper.
This presents a version of the comment reflowing with less mutable state inside
the comment breakable token subclasses. The state has been pushed into the
24 matches
Mail list logo
