This revision was automatically updated to reflect the committed changes.
Closed by commit rL319314: Restructure how we break tokens. (authored by
klimek).
Repository:
rL LLVM
https://reviews.llvm.org/D40310
Files:
cfe/trunk/lib/Format/BreakableToken.cpp
krasimir accepted this revision.
krasimir added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D40310
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
klimek added inline comments.
Comment at: lib/Format/ContinuationIndenter.cpp:1749
+ }
+ if (!Reflow) {
+// If we didn't reflow into the next line, the only space to consider
is
krasimir wrote:
> nit: Maybe change this to `if (Reflow)` and
krasimir added inline comments.
Comment at: lib/Format/ContinuationIndenter.cpp:1749
+ }
+ if (!Reflow) {
+// If we didn't reflow into the next line, the only space to consider
is
nit: Maybe change this to `if (Reflow)` and switch the if-else
klimek added inline comments.
Comment at: lib/Format/ContinuationIndenter.cpp:1707
+ RemainingTokenColumns = Token->getRemainingLength(
+ NextLineIndex, TailOffset, ContentStartColumn);
+ Reflow = true;
krasimir wrote:
> When we're
klimek updated this revision to Diff 124709.
klimek marked 4 inline comments as done.
klimek added a comment.
Address review comments.
https://reviews.llvm.org/D40310
Files:
lib/Format/BreakableToken.cpp
lib/Format/BreakableToken.h
lib/Format/ContinuationIndenter.cpp
krasimir added a comment.
Re: "I tried to write a test for this, and convinced myself that while +1 is
correct, it is currently impossible to change behavior based on the missing
+1.":
In order to have different outcome based on the start column, you could use
tabs. Consider the content
krasimir added inline comments.
Comment at: lib/Format/BreakableToken.cpp:178
Split Split) const {
// Example: consider the content
// lala lala
Offtopic: Should add a FIXME that this doesn't really work in
klimek added inline comments.
Comment at: lib/Format/ContinuationIndenter.cpp:1525
+ if (!DryRun)
+Token->adaptStartOfLine(0, Whitespaces);
+
krasimir wrote:
> If we indent here, shouldn't that also change ContentStartColumn?
Nope, this will exactly adapt
klimek updated this revision to Diff 124581.
klimek marked 3 inline comments as done.
klimek added a comment.
Address review comments.
https://reviews.llvm.org/D40310
Files:
lib/Format/BreakableToken.cpp
lib/Format/BreakableToken.h
lib/Format/ContinuationIndenter.cpp
krasimir added inline comments.
Comment at: lib/Format/ContinuationIndenter.cpp:1525
+ if (!DryRun)
+Token->adaptStartOfLine(0, Whitespaces);
+
If we indent here, shouldn't that also change ContentStartColumn?
Comment at:
klimek added a comment.
Restructured to make the invariants clearer based on a chat with Krasimir.
https://reviews.llvm.org/D40310
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
klimek updated this revision to Diff 124381.
klimek added a comment.
Restructure based on code review feedback.
https://reviews.llvm.org/D40310
Files:
lib/Format/BreakableToken.cpp
lib/Format/BreakableToken.h
lib/Format/ContinuationIndenter.cpp
unittests/Format/FormatTest.cpp
klimek added inline comments.
Comment at: lib/Format/ContinuationIndenter.cpp:1518
+ unsigned RemainingTokenColumns = 0;
+ // The column number we're currently at.
+ unsigned ContentStartColumn = 0;
krasimir wrote:
> Could you please spell out the invariants
krasimir added a comment.
Started the review. It would take a few cycles
Comment at: lib/Format/ContinuationIndenter.cpp:1518
+ unsigned RemainingTokenColumns = 0;
+ // The column number we're currently at.
+ unsigned ContentStartColumn = 0;
Could you
klimek marked an inline comment as done.
klimek added inline comments.
Comment at: lib/Format/ContinuationIndenter.cpp:1504
: Style.PenaltyBreakComment;
- unsigned RemainingSpace = ColumnLimit - Current.UnbreakableTailLength;
+ // Stores
klimek updated this revision to Diff 124095.
klimek added a comment.
Pull out getRemainingLength.
https://reviews.llvm.org/D40310
Files:
lib/Format/BreakableToken.cpp
lib/Format/BreakableToken.h
lib/Format/ContinuationIndenter.cpp
unittests/Format/FormatTest.cpp
krasimir added inline comments.
Comment at: lib/Format/BreakableToken.cpp:198
+ "Getting the length of a part of the string literal indicates that "
+ "the code tries to reflow it.");
+ return UnbreakableTailLength + Postfix.size() +
klimek
klimek added inline comments.
Comment at: lib/Format/BreakableToken.cpp:198
+ "Getting the length of a part of the string literal indicates that "
+ "the code tries to reflow it.");
+ return UnbreakableTailLength + Postfix.size() +
krasimir
klimek updated this revision to Diff 124075.
klimek marked 10 inline comments as done.
klimek added a comment.
Address review comments.
https://reviews.llvm.org/D40310
Files:
lib/Format/BreakableToken.cpp
lib/Format/BreakableToken.h
lib/Format/ContinuationIndenter.cpp
krasimir added a comment.
Here're a few nits. I'll need an evening to review the meaty part :)
Comment at: lib/Format/BreakableToken.cpp:73
+ if (ColumnLimit <= ContentStartColumn + 1) {
return BreakableToken::Split(StringRef::npos, 0);
+ }
nit: no
klimek created this revision.
This fixes some bugs in the reflowing logic and splits out the concerns
of reflowing from BreakableToken.
Things to do after this patch:
- Refactor the breakProtrudingToken function possibly into a class, so we can
split it up into methods that operate on the
22 matches
Mail list logo
