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

mydeveloperday · 2024-04-12T08:01:45Z

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 · 2024-04-12T08:06:08Z

@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 · 2024-04-12T08:14:04Z

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 · 2024-04-12T08:36:40Z

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 · 2024-04-12T09:42:19Z

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

owenca · 2024-04-14T06:24:30Z

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 · 2024-04-15T10:22:58Z

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 · 2024-04-15T12:06:00Z

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 · 2024-04-16T06:19:59Z

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 · 2024-04-16T06:39:16Z

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?

kadircet · 2024-04-16T11:31:36Z

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.

FWIW, it was mostly to dampen some big shift in defaults caused by d68826d, which completely turned off breaking between string literals. 27f5479 brought it down to cases that don't have an explicit \n in the literal.

I mostly went with this approach because there seemed to be some stance from maintainers around this new behavior being preferred, and some option based mechanism already being discussed in #69859. So I wanted to find a less-disruptive middle ground until things have finally settled.

So, maybe I misread the room back in the day, but if I felt like a revert on d68826d was an option, I'd definitely go with that rather than 27f5479 :D

Hence, I am definitely in favor of completely reverting both d68826d and 27f5479.

As for my 2cents on defaults; I'd be leaning towards preserving the compatibility with existing formatting behaviour. Even if an option makes things easier to configure (and less of a churn for my employer), it's still disruptive for the ecosystem as people use different versions of clang-format, and in such cases a version skew between different developers just makes it much harder for everyone.

Reverts commit d68826d, which changes the previous default behavior of always breaking before a stream insertion operator `<<` if both operands are string literals. Also reverts the related commits 27f5479 and bf05be5. See the discussion in llvm#88483.

owenca · 2024-04-17T04:20:50Z

Hence, I am definitely in favor of completely reverting both d68826d and 27f5479.

+1. It seems that both @mydeveloperday and @djasper would be okay with this, so I've opened a pull request for it.

…89016) Reverts commit d68826d, which changes the previous default behavior of always breaking before a stream insertion operator `<<` if both operands are string literals. Also reverts the related commits 27f5479 and bf05be5. See the discussion in #88483.

externl · 2024-05-10T13:58:13Z

Is there a setting for this in either 18.1.4 or 18.1.5?

Our CI is on 18.1.4 (The latest available in the apt packages. Any known ETA on 18.1.5 apt packages?) and the latest on Homebrew is 18.1.5. This is causing lots of CI failures for us?

Any suggestions would be appreciated.

owenca · 2024-05-15T04:37:57Z

See #92214.

externl · 2024-05-15T12:31:23Z

If I understand correctly, the 18.1.5 behavior is considered a regression?

owenca · 2024-05-16T06:58:26Z

Actually, the behavior regressed in 18.1.1 through 18.1.4 and was restored to that of 17.0.6 and earlier in 18.1.5.

owenca · 2024-07-05T06:30:27Z

This has been addressed by 8fe39e6.

github-actions bot added the clang-format label Apr 12, 2024

mydeveloperday self-assigned this Apr 12, 2024

mydeveloperday added regression bug Indicates an unexpected problem or unintended behavior labels Apr 12, 2024

mydeveloperday mentioned this issue Apr 12, 2024

[clang-format] Don't break between string literal operands of << #69871

Merged

mydeveloperday mentioned this issue Apr 12, 2024

[clang-format] revert to string << string handling to previous default #88490

Closed

owenca mentioned this issue Apr 17, 2024

[clang-format] Revert breaking stream operators to previous default #89016

Merged

owenca closed this as completed Jul 5, 2024

sonata-chen mentioned this issue Oct 30, 2024

Qt: replace QRegExp with QRegularExpression supercollider/supercollider#6512

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mydeveloperday commented Apr 12, 2024 •

edited

Loading

llvmbot commented Apr 12, 2024

mydeveloperday commented Apr 12, 2024 •

edited

Loading

djasper commented Apr 12, 2024 via email

mydeveloperday commented Apr 12, 2024

owenca commented Apr 14, 2024 •

edited

Loading

mydeveloperday commented Apr 15, 2024

djasper commented Apr 15, 2024 via email

owenca commented Apr 16, 2024

owenca commented Apr 16, 2024

kadircet commented Apr 16, 2024

owenca commented Apr 17, 2024

externl commented May 10, 2024 •

edited

Loading

owenca commented May 15, 2024

externl commented May 15, 2024

owenca commented May 16, 2024

owenca commented Jul 5, 2024

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

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

Comments

mydeveloperday commented Apr 12, 2024 • edited Loading

llvmbot commented Apr 12, 2024

mydeveloperday commented Apr 12, 2024 • edited Loading

djasper commented Apr 12, 2024 via email

mydeveloperday commented Apr 12, 2024

owenca commented Apr 14, 2024 • edited Loading

mydeveloperday commented Apr 15, 2024

djasper commented Apr 15, 2024 via email

owenca commented Apr 16, 2024

owenca commented Apr 16, 2024

kadircet commented Apr 16, 2024

owenca commented Apr 17, 2024

externl commented May 10, 2024 • edited Loading

owenca commented May 15, 2024

externl commented May 15, 2024

owenca commented May 16, 2024

owenca commented Jul 5, 2024

mydeveloperday commented Apr 12, 2024 •

edited

Loading

mydeveloperday commented Apr 12, 2024 •

edited

Loading

owenca commented Apr 14, 2024 •

edited

Loading

externl commented May 10, 2024 •

edited

Loading