klimek added a comment.
In https://reviews.llvm.org/D33589#942128, @Typz wrote:
> Indeed, looks good, thanks!
>
> Though that exacerbates the alignment issue, I now get things like this:
>
> enum {
> SomeLongerEnum, // comment
> SomeThing, // comment
> Foo, // something
> }
Typz added a comment.
Indeed, looks good, thanks!
Though that exacerbates the alignment issue, I now get things like this:
enum {
SomeLongerEnum, // comment
SomeThing, // comment
Foo, // something
}
^ (column limit)
The comment on 'Foo' would
klimek added a comment.
In https://reviews.llvm.org/D33589#941979, @Typz wrote:
> I think the difference between code and comments is that code "words" are
> easily 10 characters or more, whereas actual words (in comments) are very
> often less than 10 characters: so code overflowing by 10
Typz added a comment.
I think the difference between code and comments is that code "words" are
easily 10 characters or more, whereas actual words (in comments) are very often
less than 10 characters: so code overflowing by 10 characters is not very
frequent. whereas small words in comment
klimek added a comment.
In https://reviews.llvm.org/D33589#933746, @Typz wrote:
> with this setting, a "non wrapped" comment will actually be reflown to
> ColumnLimit+10...
Isn't the same true for code then, though? Generally, code will protrude by 10
columns before being broken?
klimek added a comment.
In https://reviews.llvm.org/D33589#933746, @Typz wrote:
> > @klimek wrote:
> > In the above example, we add 3 line breaks, and we'd add 1 (or more)
> > additional line breaks when reflowing below the column limit.
> > I agree that that can lead to different overall
Typz added a comment.
> @klimek wrote:
> In the above example, we add 3 line breaks, and we'd add 1 (or more)
> additional line breaks when reflowing below the column limit.
> I agree that that can lead to different overall outcomes, but I don't see
> how the approach of this patch really
klimek added a comment.
In https://reviews.llvm.org/D33589#933568, @klimek wrote:
> In https://reviews.llvm.org/D33589#931723, @Typz wrote:
>
> > In https://reviews.llvm.org/D33589#925903, @klimek wrote:
> >
> > > I think this patch doesn't handle a couple of cases that I'd like to see
> > >
klimek added a comment.
In https://reviews.llvm.org/D33589#931802, @Typz wrote:
> Btw, another issue I am having is that reflowing does not respect the
> alignment. For exemple:
>
> enum {
> Foo,///< This is a very long comment
> Bar,///< This is shorter
> BarBar,
klimek added a comment.
In https://reviews.llvm.org/D33589#931723, @Typz wrote:
> In https://reviews.llvm.org/D33589#925903, @klimek wrote:
>
> > I think this patch doesn't handle a couple of cases that I'd like to see
> > handled. A counter-proposal with different trade-offs is in
> >
Typz added a comment.
Btw, another issue I am having is that reflowing does not respect the
alignment. For exemple:
enum {
Foo,///< This is a very long comment
Bar,///< This is shorter
BarBar, ///< This is shorter
} Stuff;
will be reflown to :
enum {
Foo,
Typz added a comment.
In https://reviews.llvm.org/D33589#925903, @klimek wrote:
> I think this patch doesn't handle a couple of cases that I'd like to see
> handled. A counter-proposal with different trade-offs is in
> https://reviews.llvm.org/D40068.
It may be simpler (though not to my
Typz added a comment.
In https://reviews.llvm.org/D33589#924716, @klimek wrote:
> One interesting trade-off I'm running into:
> My gut feeling is that we really want to make local decisions about whether
> we want to break/reflow - this makes the code significantly simpler (IMO),
> and
klimek added a comment.
I think this patch doesn't handle a couple of cases that I'd like to see
handled. A counter-proposal with different trade-offs is in
https://reviews.llvm.org/D40068.
https://reviews.llvm.org/D33589
___
cfe-commits mailing
klimek added a comment.
One interesting trade-off I'm running into:
My gut feeling is that we really want to make local decisions about whether we
want to break/reflow - this makes the code significantly simpler (IMO), and
handles all tests in this patch correctly, but is fundamentally limiting
klimek added a comment.
In https://reviews.llvm.org/D33589#920160, @Typz wrote:
> ping ?
I'm working on understanding this better :) I've refactored the code a bit so I
could fully understand the problem, which I now do (sorry for this taking a
while, but it took me multiple hours to work
Typz added a comment.
ping ?
https://reviews.llvm.org/D33589
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Typz added a comment.
> My question is: if CanBreak is false, we currently don't call
> breakProtrudingToken. So either we do something very wrong in that case
> (which might be true, but I'd like to understand why) or we should be able
> to just calculate the penalty by not breaking anything
klimek added a comment.
In https://reviews.llvm.org/D33589#876196, @Typz wrote:
> This cannot be implemented where we currently call breakProtrudingToken(),
> since this function starts by 'creating' the BreakableToken and dealing with
> meany corner cases: so this should be done before in any
Typz added a comment.
This cannot be implemented where we currently call breakProtrudingToken(),
since this function starts by 'creating' the BreakableToken and dealing with
meany corner cases: so this should be done before in any case.
But the code at the end of breakProtrudingToken() is
klimek added a comment.
I find the current semantics of the functions a bit surprising, specifically:
... reflowProtrudingToken(..., bool Reflow)
is really confusing me :)
I'd have expected something like this where we currently call
breakProtrudingToken():
if (CanBreak) {
ReflowPenalty
Typz updated this revision to Diff 115830.
Typz added a comment.
Remove `Reflow` from LineState, and perform the decision again during
reconstruction phase.
https://reviews.llvm.org/D33589
Files:
lib/Format/ContinuationIndenter.cpp
lib/Format/ContinuationIndenter.h
Typz added a comment.
> I am still trying to get to the bottom of this assertion, any hint where to
> look for?
OK, got it. The issue is that we will actually need to run the wrapping 3 times
when DryRun = false : call reflowProtrudingToken() twice with DryRun=true to
find out the better
Typz added a comment.
In https://reviews.llvm.org/D33589#875039, @djasper wrote:
> I think doing the computation twice is fine. Or at least, I'd need a test
> case where it actually shows substantial overhead before doing what you are
> doing here. Understand that creating more States and
djasper added a comment.
I think doing the computation twice is fine. Or at least, I'd need a test case
where it actually shows substantial overhead before doing what you are doing
here. Understand that creating more States and making the State object itself
larger also has cost and that cost
Typz added a comment.
For one thing, we need to update the state to store the "decision" of the
reflowing mode, which is performed only in DryRun=true mode, to avoid doing the
computation multiple times.
Apart from this, the decision is conceptually internal to
breakProtrudingToken(). But the
djasper added a comment.
I still don't understand yet. breakProtrudingToken has basically two options:
1. Don't wrap/reflow: In this case the penalty is determined by the number of
excess characters.
2. Wrap/reflow: I this case the penalty is determined by PenaltySplitComments
plus the
Typz updated this revision to Diff 115051.
Typz added a comment.
Reorder the functions to minimize diff.
https://reviews.llvm.org/D33589
Files:
lib/Format/ContinuationIndenter.cpp
lib/Format/ContinuationIndenter.h
lib/Format/UnwrappedLineFormatter.cpp
unittests/Format/FormatTest.cpp
Typz added inline comments.
Comment at: lib/Format/ContinuationIndenter.cpp:1339
+unsigned ContinuationIndenter::breakProtrudingToken(const FormatToken ,
+LineState ,
djasper wrote:
> Can you create a patch
djasper added a comment.
I have a slightly hard time grasping what this patch now actually does? Doesn't
it simply try to decide whether or not to make a split locally be comparing the
PenaltyBreakComment against the penalty for the access characters? If so,
couldn't we simply do that as an
Typz added a comment.
ping?
https://reviews.llvm.org/D33589
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Typz updated this revision to Diff 108651.
Typz added a comment.
Rebase
https://reviews.llvm.org/D33589
Files:
lib/Format/ContinuationIndenter.cpp
lib/Format/ContinuationIndenter.h
lib/Format/UnwrappedLineFormatter.cpp
unittests/Format/FormatTest.cpp
Index:
Typz updated this revision to Diff 106442.
Typz added a comment.
Move code out of optimizer, directly into
ContinuationIndenter::breakProtrudingToken(), to minimize impact on performance.
Comment reflowing of breakable items which break the limit will be slightly
slower, since we now consider
Typz marked 2 inline comments as done.
Typz added inline comments.
Comment at: lib/Format/UnwrappedLineFormatter.cpp:723
FormatDecision LastFormat = Node->State.NextToken->Decision;
if (LastFormat == FD_Unformatted || LastFormat == FD_Continue)
+
Typz added inline comments.
Comment at: lib/Format/UnwrappedLineFormatter.cpp:723
FormatDecision LastFormat = Node->State.NextToken->Decision;
if (LastFormat == FD_Unformatted || LastFormat == FD_Continue)
+addNextStateToQueue(Penalty, Node,
djasper added inline comments.
Comment at: lib/Format/UnwrappedLineFormatter.cpp:723
FormatDecision LastFormat = Node->State.NextToken->Decision;
if (LastFormat == FD_Unformatted || LastFormat == FD_Continue)
+addNextStateToQueue(Penalty, Node,
Typz added a comment.
ping?
https://reviews.llvm.org/D33589
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Typz added a comment.
In https://reviews.llvm.org/D33589#789002, @alexfh wrote:
> why do we want to make an exception for comments and not for regular code?
This is not an exception for comments: the `PenaltyExcessCharacter` is used
whenever the code is longer than the `ColumnLimit`, and used
alexfh added a comment.
Daniel is a better reviewer here than myself. A few cents from me though: why
do we want to make an exception for comments and not for regular code?
https://reviews.llvm.org/D33589
___
cfe-commits mailing list
Typz added a comment.
@krasimir, @alexfh : can I get some feedback?
This patch solves a practical problem, i.e. allowing the comment to overflow a
bit without triggering a reflow [according to the priorities which are in
config, obviously]. It may not always provide the "best" wrapping, but as
Typz added a comment.
@krasimir : ping
https://reviews.llvm.org/D33589
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Typz added a comment.
ping?
https://reviews.llvm.org/D33589
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Typz added a comment.
In https://reviews.llvm.org/D33589#769893, @krasimir wrote:
> I think that what you're trying to solve is not practically that important,
> is unlikely to improve the handling of comments, and will add a lot of
> complexity.
Not sure the 'approach' I have in this patch
krasimir added a comment.
I think that what you're trying to solve is not practically that important, is
unlikely to improve the handling of comments, and will add a lot of complexity.
From a usability perspective, I think that people are happy enough when their
comments don't exceed the line
Typz updated this revision to Diff 100698.
Typz marked an inline comment as done.
Typz added a comment.
fix code & tests
https://reviews.llvm.org/D33589
Files:
lib/Format/ContinuationIndenter.cpp
lib/Format/ContinuationIndenter.h
lib/Format/FormatToken.cpp
lib/Format/FormatToken.h
Typz marked an inline comment as done.
Typz added inline comments.
Comment at: unittests/Format/FormatTest.cpp:8571
+"*/",
+format("int a; /* first line second line third line */", Style));
+}
Typz wrote:
> This is not working as
Typz added a comment.
Still some issues with the patch, I would need some feedback first:
- Is this approach desirable, as a relatively easy fix?
- Or should this be fixed with a complete refactoring of the way the
strings/comments are split, making multiple tokens out of them to let them be
Typz updated this revision to Diff 100379.
Typz added a comment.
fix indentation issues
https://reviews.llvm.org/D33589
Files:
lib/Format/ContinuationIndenter.cpp
lib/Format/ContinuationIndenter.h
lib/Format/FormatToken.cpp
lib/Format/FormatToken.h
Typz created this revision.
Herald added a subscriber: klimek.
This patch tries to improve the optimizer a bit, to avoid splitting
tokens (e.g. comments/strings) if only there are only few characters
beyond the ColumnLimit.
Previously, comments/strings would be split whenever they went beyond
49 matches
Mail list logo
