[PATCH] D33589: clang-format: consider not splitting tokens in optimization

[Manuel Klimek via Phabricator via cfe-commits]

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 handled. A counter-proposal with different trade-offs is in https://reviews.llvm.org/D40068.
> >
> >
> > Can you provide more info on these cases? Are they all added to the tests of https://reviews.llvm.org/D40068?
> >
> > It may be simpler (though not to my eyes, I am not knowledgeable enough to really understand how you go this fixed...), and works fine for "almost correct" comments: e.g. when there are indeed just a few extra characters overall. But it still procudes strange result when each line of the (long) comment is too long, but not enough to trigger a line-wrap by itself.
>




>> Since that version has landed already, not sure how to improve on this. I could probably rewrite my patch on master, but it seems a bit redundant. As a simpler fix, I could imagine adding a "total" overflow counter, to allow detecting the situation; but when this is detected (e.g. on subsequent lines) we would need to "backtrack" and revisit the initial decision...
> 
> Yes, I added all tests I was thinking of to the patch. Note that we can always go back on submitted patches / do things differently. As my patch fulfilled all your tests, I (perhaps incorrectly?) assumed it solved your use cases - can you give me a test case of what you would want to happen that doesn't happen with my patch?
> 
> Are you, for example, saying that in the case
> 
>   Limit; 13
>   // foo bar baz foo bar baz foo bar baz
>   you'd not want
>   // foo bar baz
>   // foo bar baz
>   // foo bar baz
>   If the overall penalty of the protruding tokens is - what? More than 1 break penalty? If you care about more than 3x break penalty (which I would think is correct), the local algorithm will work, as local decisions will make sure the overall penalty cannot be exceeded.

Ok, sorry, after reading the comment on the other patch I get it :)
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 fixes it - it will only ever reflow below the column limit, so it'll also lead to states for long lines where reflowing and leaving chars over the line limit might be the overall best choice (unless I'm missing something)?


https://reviews.llvm.org/D33589