[clang-format] change to string << string handling causes huge code changes

[mydeveloperday]

The change made for #44363 causes massive amount of change in the default behaviour, we shouldn't change the defaults like this without an option.

[llvmbot]

@llvm/issue-subscribers-bug

Author: MyDeveloperDay (mydeveloperday)

The change made for #44363 causes massive amount of change in the default behaviour, we shouldn't change the defaults like this without an option.

[mydeveloperday]

We should not change the default behaviour unless this the original cause was a bug (which it wasn't in this case it was intentional), this was a conscious decision made by @djasper (I assume) and whilst I know it might seem odd sometimes when we applied this to our large base the impact is huge and not necessarily for the better.

While I appreciate it goes wrong for some it doesn't mean its bad always:

osjson << "{\n"
       <<"  \"name\": \"value\",\n"
       <<"  \"key\": \"abc\", \n"
       <<" }";

now becomes

osjson << "{\n" <<"  \"name\": \"value\",\n <<"  \"key\": \"abc\",\n  << "}";

While i understand its correct for column width its not as nice to read! Its not even just about detecting the "\n" as this is equally valid reason to have it formatted the way you want to.

osjson << "{"
       <<"  \"name\": \"value\","
       <<"  \"key\": \"abc\","
       <<" }";

These rules about breaking between << were part of the original authors rules to best try and format c++ and I sort of feel we should at least honour it via an option. (and that option should be the default)

I'm proposing here we make a new option (as an enum so we can add other behaviours in the future)

enum BreakChevronOperator
{
     Never
     BetweenStrings
}

But the default needs to be BetweenStrings.. I've spent years defending that "we don't change the defaults in clang-format, we only fix bugs", this goes against that, as this original behvaiour wasn't a bug.

[djasper]

FWIW, breaking between two consecutive string literals on either side of a "<<" makes sense to me.

…

On Fri, Apr 12, 2024 at 10:15 AM MyDeveloperDay ***@***.***> wrote: We should not change the default behaviour unless this the original cause was a bug (which it wasn't in this case it was intentional), this was a conscious decision made by @djasper <https://github.com/djasper> (I assume) and whilst I know it might seem odd sometimes when we applied this to our large base the impact is huge and not necessarily for the better. While I appreciate it goes wrong for some it doesn't mean its bad always: osjson << "{\n" <<" \"name\": \"value\",\n" <<" \"key\": \"abc\", \n" <<" }"; now becomes osjson << "{\n" <<" \"name\": \"value\",\n <<" \"key\": \"abc\",\n << "}"; While i understand its correct for column width its not as nice to read! Its not even just about detecting the "\n" as this is equally valid reason to have it formatted the way you want to. osjson << "{" <<" \"name\": \"value\"," <<" \"key\": \"abc\"," <<" }"; These rules about breaking between << were part of the original authors rules to best try and format c++ and I sort of feel we should at least honour it via an option. (and that option should be the default) I'm proposing here we make a new option (as an enum so we can add other behaviours in the future) enum BreakChevronOperator { Never BetweenStrings } But the default needs to be BetweenStrings.. I've spent years defending that "we don't change the defaults, we only fix bugs"... this original behvaiour wasn't a bug. — Reply to this email directly, view it on GitHub <#88483 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAODZ44MMUZKRMVZYZLHAPTY46JYVAVCNFSM6AAAAABGDVOFR2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJRGI2DOMBVHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[mydeveloperday]

A rare appearance from @djasper ;-) means we have to fix this!!!

[owenca]

We should not change the default behaviour unless this the original cause was a bug (which it wasn't in this case it was intentional), this was a conscious decision made by @djasper (I assume) and whilst I know it might seem odd sometimes when we applied this to our large base the impact is huge and not necessarily for the better.

While I appreciate it goes wrong for some it doesn't mean its bad always:

osjson << "{\n"
       <<"  \"name\": \"value\",\n"
       <<"  \"key\": \"abc\", \n"
       <<" }";

now becomes

osjson << "{\n" <<"  \"name\": \"value\",\n <<"  \"key\": \"abc\",\n  << "}";

See #69871 (comment).

I'm proposing here we make a new option (as an enum so we can add other behaviours in the future)

enum BreakChevronOperator
{
     Never
     BetweenStrings
}

See #69859 (comment).

But the default needs to be BetweenStrings.

The default behavior has been "breaking between two adjacent string literals only if the first one ends with a newline" since clang-format 18, and this should be the default value of whatever new option we add for breaking string literals.

this original behvaiour wasn't a bug.

People who commented on #44363 and clangd/vscode-clangd#545 thought otherwise. See also #69871 (review).

[mydeveloperday]

I've read everything about this,I even proposed a similar change year ago, I just wish we hadn't changed the default... I have no problem with the change or the additional change to support "\n" but I just wish it had gone in as an option. It was never a bug, but was intended to behave like that. Now 1 or 2 people are making the decision to change it for everyone.

The question really comes down to if you have two strings next to each other

os << "ABC" << "DEF"

why not do it as os << "ABCDEF" if you want them on one line, and so hence if you have a "<<" between two literals then you want some form of a break between them. Hence the logical breaking (regardless of the newline or not)

os << "ABC"
<< "DEF"

At the end of the day, when 18 hits people source (likely most users are not on 18) we are going to generate alot of flux.. at that point I think we'll get people coming back saying clang-format keep changing the defaults.

[djasper]

Regarding these two things individually: Does the line wrapping make sense: I have looked across Google's codebase and almost all insteads of having <literal> << <literal> without a line break was caused by clang-format merging lines that the user had previously split. As such, I agree that this is very likely the desired behavior. Rolling out / changing defaults: If a separate option for this is introduced, I think we should keep the current behavior as default to cause less churn. However, this specific churn is IMO also acceptable, i.e. if we chose to not even make this an additional option. The trade-off here is causing churn vs. ever increasing the codebase and configuration complexity. I am not entirely certain that creating a new option here is worth its weight. But that's just my 2c, don't take that to mean too much (and I have not read up on the full history on this particular behavior).

…

On Mon, Apr 15, 2024 at 12:23 PM MyDeveloperDay ***@***.***> wrote: I've read everything about this,I even proposed a similar change year ago, I just wish we hadn't changed the default... I have no problem with the change or the additional change to support "\n" but I just wish it had gone in as an option. It was never a bug, but was intended to behave like that. Now 1 or 2 people are making the decision to change it for everyone. The question really comes down to if you have two strings next to each other os << "ABC" << "DEF" why not do it as os << "ABCDEF" if you want them on one line, and so hence if you have a "<<" between two literals then you want some form of a break between them. Hence the logical breaking (regardless of the newline or not) os << "ABC" << "DEF" At the end of the day, when 18 hits people source (likely most users are not on 18) we are going to generate alot of flux.. at that point I think we'll get people coming back saying clang-format keep changing the defaults. — Reply to this email directly, view it on GitHub <#88483 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAODZ47A3FY6NEZHHDNJVL3Y5OTBTAVCNFSM6AAAAABGDVOFR2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJWGQ3TOMBQGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[owenca]

I've read everything about this,I even proposed a similar change year ago, I just wish we hadn't changed the default... I have no problem with the change or the additional change to support "\n" but I just wish it had gone in as an option. It was never a bug, but was intended to behave like that. Now 1 or 2 people are making the decision to change it for everyone.

This change was intended as a bug fix, not a change to the LLVM default. It has been baked on the main branch for 5+ months and gone into release 18. Had anyone wanted an option instead, either in the review of #69871 or in a request before 18 was branched, d68826d would not have landed or would have been reverted before 18.1.1.

[owenca]

Rolling out / changing defaults: If a separate option for this is introduced, I think we should keep the current behavior as default to cause less churn. However, this specific churn is IMO also acceptable, i.e. if we chose to not even make this an additional option. The trade-off here is causing churn vs. ever increasing the codebase and configuration complexity. I am not entirely certain that creating a new option here is worth its weight. But that's just my 2c, don't take that to mean too much (and I have not read up on the full history on this particular behavior).

A third option would be to revert both d68826d and 27f5479 before 18.1.5 is released as I doubt that we can fix most of the existing issues related to breaking "<<" without a new option. @kadircet @HazardyKnusperkeks @rymiel would you please weigh in?