Is this written down somewhere? https://webkit.org/code-style-guidelines/ doesn't seem to mention it.

LG

In D45726#1071925, @krasimir wrote:

Another point: for the example in the summary about bailing-out early, is there a test for this already? If not, we should add one.

Address comments.

lg

lg

Just from a formatting point of view, why not:

//.   Comment
#.    define X

In D37813#958116, @Typz wrote:

OK.

So you mean a solution like the one discussed earlier would be the way to go?

I mean that we can configure macros in the format style, like "define A(X) class X {". I'm not 100% sure whether we would just try to use the Preprocessor for this, or whether we'd want to only allow a small subset of actual macros, but the general idea would be the same: The UnwrappedLineParser would parse the macro at the expansion location A(X) into an unwrapped line, and then parse the expansion into a child line, with the tokens tha tare not in the argument of the call being marked as fixed (parent child might also be better inverted).

(As a side-note, I want to stress out that we would actually need a 'reversible' description to support the namespace case, to allow generating the end-of-namespace comment)

I've talked with Daniel and we both believe this patch is not the right way to go forward for clang-format. It is adding configuration mechanisms that we need to maintain going forward, without addressing a general problem; specifically for how to handle macros, I believe there is a clear better way to do it that is also fundamentally more useful than adding extra knobs for every problem we encounter. Finally, these changes do not address a fundamental bug, and workarounds exist, so there is no large benefit to do this now in a non-principled way rather than wait for a more principled fix. Overall, I do understand that this is a trade-off, but I hope you understand that we sometimes have to make decisions.

lg

lg

lg

In D37813#945125, @Typz wrote:

I don't think this is really relevant for this tool: if someone changes the implementation of the macro, then *they* must indeed if it should not be formatted like a namespace (and keep the clang-format configuration unchanged), or if it should now be formatted like a class (and thus make changes to clang-format configuration). Here we are not defining what the macro does, but how clang-format should indent it : in most case I don't think the indentation format should actually depend on the way it is implemented...

In D37813#942137, @Typz wrote:

As far as "parsing" and formatting inside the block is concerned, this is indeed unrelated (and would totally work if all macros where specified with some preprocessor definitions).

But identifying the 'opening' token and generating the matching 'closing' comment are totally related; it would seem very strange to have an option to specify that TESTSUITE() macros are parsed as namespace, then another option to indicate that namespace declared by this macros are actually closed with another macro call...

In 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
} 
                            ^ (column limit)

The comment on 'Foo' would overflow a bit, but it gets unindented during "alingment" stage, which kind of 'breaks' the decision that was made earlier on *not* to break the comment...

In D37813#941987, @Typz wrote:

Definitely that would be much more expressive. But this approach is also incomplete: in case of namespace (and possibly others?), we also need to perform the reverse operation, e.g. to "generate" a macro call for rewriting the closing comment.

On top of this, I think that is really not trivial, I would prefer to move forward with these "simpler" patch, and change the whole macro configurations part in later commits...

In 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 characters is not very frequent. whereas small words in comment will often get closer to the "extra" limit.

That said, I tried with your latest change ("Restructure how we break tokens", sha1:64d42a2fb85ece5987111ffb908c6bc7f7431dd4). and it's working about fine now. For the most part it seems to wrap when I would expect it, great work!
I have seen 2 "issues" though:

• Often I see that the last word before reflowing is not wrapped (eventhough it overlaps the line length); I did not count penalties so I cannot confirm this is really an issue or just a borderline scenario.
• Alignment seems better than before, but since there is no penalty for breaking alignment it will always try to unindent to compensate for overflowing characters...
  
  Seeing this, I guess this patch does not make much sense anymore, I'll see if I make some improvements for these two issues, in separate patches.

Add test.

Fix incorrect return value leading to unnecessary computation.

Address review comments.

Address review comments.

LG

LG. Thx!

Restructured to make the invariants clearer based on a chat with Krasimir.

Restructure based on code review feedback.

Self-accepting and closing, as the underlying change has landed, and this diff is incorrect now.
I also have an idea to solve the problem Typz has brought up.

Pull out getRemainingLength.

In D33589#933746, @Typz wrote:

with this setting, a "non wrapped" comment will actually be reflown to ColumnLimit+10...

In 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 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)?

Definitely, this patch may not find the 'optimal' solution. What I mean is that we reduce the PenaltyExcessCharacter value to allow "occasionally" breaking the limit: instead of a hard limit, we want to allow lines to sometimes break the limit, but definitely not *all* the time. Both patch work fine when the code is "correct", i.e. there is indeed only a few lines which break the limit.

But the local decision approach behaves really wrong IMHO when the code is formatted beyond the column: it can very easily reformat in such a way that the comment is reflown to what looks like a longer column limit. I currently have a ratio of 10 between PenaltyBreakComment and PenaltyExcessCharacter (which empirically seemed to give a decent compromise, and match how our code is formatted; I need to try more with your patch, to see if we can get better values...): with this setting, a "non wrapped" comment will actually be reflown to ColumnLimit+10...

In D37813#930065, @Typz wrote:

ping?

Address review comments.

In D33589#933568, @klimek wrote:

In D33589#931723, @Typz wrote:

In 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 D40068.

Can you provide more info on these cases? Are they all added to the tests of 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.

In D40068#931679, @Typz wrote:

Generally, this indeed improves the situation (though I cannot say much about the code itself, it is still too subtle for my shallow knowledge of clang-format).

But it seems to give some strange looking result with long comments: it seems like the decision is made at each line (e.g. is it better to wrap this line or overflow a bit), so we can get a comment where each line overflows by a few characters, even if the total is worse... For exemple, say we have a 100 lines of comment, with 9 characters overflow on each line, and an excess character penalty of 30 : with this patch nothing will be re-wrapped (9*30 = 270 is less the the 300 penalty for wrapping); but the total penatly would be 900....

(btw, it seems this got merged, but the ticket does not reflect it)

In 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, ///< This is shorter
} Stuff;

will be reflown to :

enum {
   Foo, ///< This is a very long
        ///< comment
   Bar, ///< This is shorter  
   BarBar, ///< This is shorter
} Stuff;

when I would expect:

enum {
   Foo,    ///< This is a very
           ///< long comment
   Bar,    ///< This is shorter  
   BarBar, ///< This is shorter
} Stuff;

I see there is no penalty for breaking alignment, which may be accounted for when compressing the whitespace 'before' the comment... Or maybe simply the breaking should be smarter, to try to keep the alignment; but I am not sure it can be done without another kind of 'global' optimization of the comment reflow... Any idea/hint?

In D33589#931723, @Typz wrote:

In 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 D40068.

Can you provide more info on these cases? Are they all added to the tests of 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...

Just exporting

LG

Address review comments.

Should that be configured? METADATA seems pretty domain specific ;)

Address review comments.

LG

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 D40068.

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 the global optimizations we can do. Specifically, we would not correctly reflow this:

//       |< limit
// foo bar
// baz
// x

to

// foo
// bar
// baz x

when the excess character limit is low.

In D33589#920160, @Typz wrote:

ping ?

In D39900#921885, @krasimir wrote:

Maybe we should further refactor getRawStringStyle into llvm::Optional<std::pair<style, delimiter>> getRawStringStyleAndDelimiter and that would nicely take care of the duplicated effort?

In D32478#920345, @Typz wrote:

Unless I'm missing something, I'd agree with Daniel; this is not a rule that's widely used, and I'd say reformatting a code base to the clang-formatted variant will not regress readability.

Unfortunately coding rules are not just about readability, but also about consistency at entreprise scale, thus as a general rule we should not change the established rules in an organizatoin.
There is history in rules, we have code which already uses these rules, so for consistency we must stick to the old rules (even if we would not necessarily choose the same rules if we were to start from scratch).

• Add test.

LG.

In D39836#920313, @sammccall wrote:

In D39836#920306, @ilya-biryukov wrote:

I personally think they're useful to have anyway, and they don't add much noise when they're at the end of completions list.
I sometimes want to complete an item I know is there and then fix accessibility. Do you think they add too much noise?

For me the noise definitely outweighs the value, e.g. when I'm trying to see all the available functions. There are costs beyond noise:

• they bulk up tests and debugging output
• they complicate the scoring scheme - we *never* want them to appear above a legal completion, so a 1-dimensional score needs special handling as here.
• compute: we spend time scoring them, computing their strings, writing them over the wire, and then the client has to process them

In D32478#920275, @Typz wrote:

Sorry for the long response time. I don't see this style rule expressed explicitly in the Skia or QtCreator style guides - is this something that just happens to be done sometimes in the code bases?

This is present in code base. Those project's style guides are not precise enough, and do not document the behavior for this case.

I have problems understanding the rules from the discussion (as well as the code / test tbh), is there a really concise way to give me the core of the idea?

The case I am trying to address is to really keep the _operands_ aligned after an assignment or return, in case line is wrapped *before* the operator.

int a = b
      + c;
return a
     + b;

while the current behavior with Style.AlignOperands = true; BreakBeforeBinaryOperators = true is to align the wrapped operator with the previous line's operand:

int a = b
        + c;
return a
       + b;

In the discussion, it appeared that this behavior is not a error (with respect to the name), but an expected behavior for most coding rules: hence the introduction of a third AlignOperands mode ("AlignAfterOperator"), to handle this new case.

From there the code actually got a bit more complex to support various corner cases (e.g. operators with different number of characters, wrapperd first line, do not unindent more than the enclosing brace...)

lg