Reconsidering our optional-brace policy

[Peter Boström]

Hi folks,

I'd like to start a discussion on changing our brace policy for single-line ifs, and I assume this discussion is brought up every N years but I'd like to bring it up again. Specifically I'd like to transform:

if (foo)

  bar();

into:

if (foo) {

  bar();
}

Personally I'm a fan of the single-line ifs omitting braces, but I'm a bigger fan of code style that's enforceable by clang-format that has a shot at being globally consistent. Just like I don't want to think about indentation, I also want to stop considering whether the current file uses if (foo) bar(); or if (foo) { bar(); } as we currently permit both but should be locally consistent.

The style guide recommends always using braces, but does allow an exception for historical reasons which Chromium does make use of: "For historical reasons, we allow one exception to the above rules: if an if statement has no else or else if clauses, then the curly braces for the controlled statement or the line breaks inside the curly braces may be omitted if as a result the entire if statement appears on either a single line (in which case there is a space between the closing parenthesis and the controlled statement) or on two lines (in which case there is a line break after the closing parenthesis and there are no braces). For example, the following forms are allowed under this exception." and we are explicitly allowed to not make use of this exception: "Some projects require curly braces always."

I would like for us to stop making use of this exception, and update the Chromium .clang-format style to include the "InsertBraces" option (and also upstream it to the clang-format "Chromium" style). To try this out, I applied it to //base here. Patchset 1 is just applying clang-format without the .clang-format change to include InsertBraces so that the diff only concerns InsertBraces.

Applying clang-format to //base caused the following build errors:

* One macro instance in windows_types.h got a redefinition warning because {0} got changed to { 0 } or something like that.
* Reordering windows.h caused compile errors

* Formatting check_unittest.h breaks because __LINE__ changes for a macro invocation.

None of these errors look related to InsertBraces and reverting these changes caused the build to pass trybots. I believe this means that it's feasible to apply InsertBraces at scale. I'm happy splitting out the style decision from whether to actually apply clang-format retroactively, but if reformatting is a prerequisite for this change I'd be happy to own this transform.

One additional benefit from InsertBraces is that it's also able to fix existing style violations which I've spotted in multiple files (if-else without brackets): https://chromium-review.googlesource.com/c/chromium/src/+/4031901/5/base/allocator/partition_allocator/page_allocator.cc

Hopefully this means we can paint our bikeshed red, even if you'd prefer for it to be blue, because a machine can paint and keep it red for us in the future, so we can focus on the bikeshed interior. WDYT?

Best,

Peter

[K. Moon]

I don't like the current "relaxed" rule, as it requires manual intervention in many cases to format correctly. +1 to adopting the stricter rule.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sHfmFDrNt4UyGT5ZTGNa9WhhifmfVhdgLpxYiE2br3ukA%40mail.gmail.com.

[Peter Kasting]

On Mon, Nov 21, 2022 at 12:29 PM 'Peter Boström' via cxx <c...@chromium.org> wrote:

Personally I'm a fan of the single-line ifs omitting braces, but I'm a bigger fan of code style that's enforceable by clang-format that has a shot at being globally consistent. Just like I don't want to think about indentation, I also want to stop considering whether the current file uses if (foo) bar(); or if (foo) { bar(); } as we currently permit both but should be locally consistent.

FWIW this is not something I ask for even local consistency on. I think the right answer is to Not Care At All Ever about this.

PK

[K. Moon]

As an example of the chief reason I see the historical rule as a problem, I've been asked to change code like this:

if (multiline &&
    condition)
  return b;

to

if (multiline &&
    condition) {
  return b;
} 

in order to comply with the historical rule. Then if the condition becomes shorter later, I may be asked to take the braces back off again (for consistency, I suppose):

if (single line condition)
  return b;

This is pointless churn, in my opinion. Let's just let the formatter handle it; any extra consistency is just a bonus.

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDs%2BKoQ_-SAcCk0YFqHYkYtEfQmxuHWMApNZ-zCKhuFkA%40mail.gmail.com.

[Peter Kasting]

On Mon, Nov 21, 2022 at 12:37 PM K. Moon <km...@chromium.org> wrote:

Then if the condition becomes shorter later, I may be asked to take the braces back off again (for consistency, I suppose):

if (single line condition)
  return b;

This is pointless churn, in my opinion.

Agree. You shouldn't be asked to do this second thing. Feel free to push back against reviewers who ask for this?

PK 

[Peter Boström]

FWIW I think the easiest way to get folks to Not Care At All Ever about this is to have the tool care for you. I don't think people will stop "nit: braces for consistency with file" on code reviews even if we wish they wouldn't. Even without reviewers nitpicking you will have folks looking at local formatting and trying to be consistent (especially when new to the codebase and rules). Presumably folks will stop thinking about that really fast if clang-format undoes whatever formatting attempt they've done.

(Also that the rule can fix existing style violations for if-else and multiline stuff is imo worth it.)

[Alexei Svitkine]

+1 to just using {}'s consistently and saving review time about it in the future if it's part of 'git cl format'.

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sG%2BvwyH_0mB%2BjD_zcCBz9trc6FtNAjmG-v1z_WjG856fA%40mail.gmail.com.

[Peter Kasting]

On Mon, Nov 21, 2022 at 12:39 PM Peter Boström <pb...@google.com> wrote:

FWIW I think the easiest way to get folks to Not Care At All Ever about this is to have the tool care for you.

The tool should always add braces if we need them; if it doesn't, it's a bug in the tool.  Let's Please Fix That regardless.

Then the tool does care for you to the extent that anything matters ever. If people are still nitting about this, I will send out a missive explicitly saying Not To Do That. I will be fairly surprised if the combo of all those things doesn't significantly reduce whatever frustration you're feeling here.

PK

[K. Moon]

I concur that having the formatter handle this is superior to pushing back on reviewers. For one thing, the reviewers are often right about inconsistency, so you at least need to expend additional mental effort having those discussions about whether it really matters or not. We could reduce this to Zero instead of Some Small Number.

I'm not entirely sure what the selling point of allowing the braceless style is at this point, other than saving one vertical line for the terminal "}". That doesn't seem like it has enough value to avoid automating the formatting. I think the onus should be on the historical rule to show that it's valuable enough not to adopt the modern rule.

[Peter Kasting]

On Mon, Nov 21, 2022 at 12:45 PM K. Moon <km...@chromium.org> wrote:

I concur that having the formatter handle this is superior to pushing back on reviewers. For one thing, the reviewers are often right about inconsistency, so you at least need to expend additional mental effort having those discussions about whether it really matters or not. We could reduce this to Zero instead of Some Small Number.

We're both proposing a world where "the formatter handles this" sufficient to be compliant with the style guide. There are other cases where the style guide and the formatter allow multiple styles (IIRC, one is "for functions taking multiple lines of args, one arg per line or multiple args per line"); I don't recall reports that reviewer pushback about inconsistency is onerous in these cases. Perhaps the issue is not that the formatter must forcibly pick one, but that (a) this rule was changed quite recently, and (b) the formatter is currently buggy and thus doesn't actually make code comply with the rule?

I'm not entirely sure what the selling point of allowing the braceless style is at this point, other than saving one vertical line for the terminal "}". That doesn't seem like it has enough value to avoid automating the formatting. I think the onus should be on the historical rule to show that it's valuable enough not to adopt the modern rule.

Besides the fact that "a lot of Chromium does it this way", the selling point is primarily that this is the common case for conditionals, and that the extra {} are line noise that papercuts readability. It is a small but reasonable win overall to be able to express common, simple conditionals as tersely and simply as possible. (See also "irregular verbs" in human languages.)

PK 

[K. Moon]

The readability argument is subjective; I personally don't find the style without braces any easier to read, and if anything, I find it harder to read because it's different from all the other conditionals that do use braces.

The "for historical reasons" language in the style guide implies that the arbiters don't think there's a sufficiently good argument for the more terse style other than consistency (otherwise, they wouldn't rely on a purely historical argument for keeping it).

I'm all for fixing clang-format to handle the historical rule if possible, but that seems like a harder hill to climb than just dropping the historical formatting.

[Alan Cutter]

As much as I like the slick look of braceless ifs I would rather have braces always for practical reasons; it would let me blindly s/return/LOG(ERROR) << "here"; return/ in a function to find out where it exits.

[Joel Hockey]

+1 to always using braces.

I agree that readability is subjective, but count me as one who finds it easier to always have them.

I relate to Alan's point quite often where I add braces into existing code in order to add debug printfs.

For me, the biggest reason to always use braces is to avoid certain types of bugs.  This is well explained at go/totw/labs/braces-on-if-statements.md (sorry non-googlers, it mentions #gotofail and shows other ways that code merges, extra / missing semicolons, and other things can surprise you)

    

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/a32c5b15-5849-442f-8a4d-0016fad1ec0an%40chromium.org.

[David Munro]

Another +1 for braces, I also find it easier to always have them (and braceless I'd usually add whitespace anyway for vertical separation), I've also seen bugs which could've been avoided by braces but never the opposite. In the past couple months I've seen: a second statement added to a braceless if without braces also being added, a braceless if which got longer so the statement was split in two lines, a braceless if which got an else added. All of these took up time in CLs to be spotted and the author to change.

Cheers,

Dave.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJJNyZZXb0kqem8TyahJ9pJDqfU0Aoa6okLGb8MQAV-P-u2%2BdA%40mail.gmail.com.

[Daniel Cheng]

I support using {}s universally and enabling the new InsertBraces clang-format option for enforcement.

However, I am less enthusiastic of re-clang-formatting the codebase. In the end, hitting reformatting CLs in blame/history is a "constant" amount of pain, but it's just one more thing that makes life harder. The older if statements that I'm likely to have questions about are also the ones that are more likely to have braces inserted.

If this could be handled better by our tooling of course...

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGSB53FQ_8-5p3JxUmFahw0zGjJ3PjeFnqJowZiBXBb-r13syw%40mail.gmail.com.

[Michael Ershov]

+1 to always using braces. 

[Ken Okada]

+1 to always using braces.

Human brain is the most expensive resource. No exception and automation makes things simple.

Let's consume more disk space!

[dan...@chromium.org]

I also prefer not using {} but I see the value in doing it still, like others in this thread.

On Mon, Nov 21, 2022 at 3:39 PM 'Peter Boström' via cxx <c...@chromium.org> wrote:

FWIW I think the easiest way to get folks to Not Care At All Ever about this is to have the tool care for you.

I was going to say this but Peter said it better so +1 that.

 

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sG%2BvwyH_0mB%2BjD_zcCBz9trc6FtNAjmG-v1z_WjG856fA%40mail.gmail.com.

[Joe Mason]

+1 to making sure clang-format always does a deterministic thing - no matter what it is - with braces, and then letting the formatter handle it from then on.

Since adding `InsertBraces` is the easiest way to implement that, and I don't think any readability advantages of a mixed style are worth the time it would take to do a more complicated thing, let's use `InsertBraces`.

I don't think we should reformat anything at large scale. Just let clang-format update code gradually as patches are written. (Although individual file owners could reformat files they own to avoid unrelated churn in patches, if they want.)

As a side note, I think we should fix the specific clang-format issues you found in //base, even if we don't apply clang-format yet, so that the files are resilient to clang-format being run on them incrementally later:

* One macro instance in windows_types.h got a redefinition warning because {0} got changed to { 0 } or something like that. 

That line should be wrapped in `// clang-format off` / `// clang-format on`. Do you mind adding that now in case clang-format ever gets run on windows_types.h?

* Formatting check_unittest.h breaks because __LINE__ changes for a macro invocation.

It looks like ScopedCheckExpectation relies on the actual CHECK or DCHECK macro to be on the last line of the EXPECT macro. (So there are a bunch of existing macros with `EXPECT_CHECK("(newline) (newline) (newline)", CHECK(foo)` that work fine, but `EXPECT_CHECK("something", CHECK(foo && (newline) bar)` won't.

This seems really fragile, so ideally the test could be updated somehow - but maybe it's enough to wrap the whole file in "clang-format off" with a comment that it depends on precise line wrapping?

* Reordering windows.h caused compile errors 

 This sounds like an issue in clang-format itself. It should know to special-case windows.h. Can you file a bug if there isn't one already?

[Bruce Dawson]

We could updated clang-format so that it knows about Windows.h, although it would probably take some experimentation to figure out exactly what the rules should be. I guess "Windows.h is allowed to go anywhere" would work.

However we've previously handled this by putting includes of windows.h in a separate block so that clang-format leaves them alone. See crrev.com/c/4048781 for a patch that would fix this issue in all of base. Thoughts?

[Peter Kasting]

On Tue, Nov 22, 2022 at 10:13 AM 'Bruce Dawson' via cxx <c...@chromium.org> wrote:

However we've previously handled this by putting includes of windows.h in a separate block so that clang-format leaves them alone. See crrev.com/c/4048781 for a patch that would fix this issue in all of base. Thoughts?

LGTM; added bonus: make sure this is documented somewhere, e.g. Dos and Don'ts list.

In any case: there seems to be clear consensus to make the core style change here. Go forth and do it, pbos@.

PK 

[Peter Boström]

Thank you all. I've filed crbug.com/1392808 and will submit a .clang-format change soon if no one shouts at me.

[Peter Boström]

For this list as well, when this change + documentation has landed I'll send out an ask/proposal for if a mass-clang-format is attractive or not (fwiw I'm in favor even with our tools' treatment of them). If we have more yay's than nay's I think that should turn into a LSC proposal.

[K. Moon]

I'm glad to see there's generally consensus here, but given that it's a holiday week and a lot of people are out, perhaps we should wait until next week before calling it?

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sG_tCykFeVRogAEutJgDH%2Bccm-CNhRGt5WAETufckMdcg%40mail.gmail.com.

[Peter Boström]

Sounds reasonable, I'll put out crrev.com/c/4048911 for review on Tuesday next week unless I hear otherwise.

[Alan Cutter]

On Tuesday, 22 November 2022 at 6:43:23 pm UTC+11 Daniel Cheng wrote:

I support using {}s universally and enabling the new InsertBraces clang-format option for enforcement.

However, I am less enthusiastic of re-clang-formatting the codebase. In the end, hitting reformatting CLs in blame/history is a "constant" amount of pain, but it's just one more thing that makes life harder. The older if statements that I'm likely to have questions about are also the ones that are more likely to have braces inserted.

I'm not in favour of rewriting the entire codebase to match if we adopt always braces. It's not worth that amount of git blame pain. Let's only have apply this to modified code.

[PhistucK]

> It's not worth that amount of git blame pain.

Does this not work well?

https://source.chromium.org/chromium/chromium/src/+/main:.git-blame-ignore-revs

☆PhistucK

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/f79f795d-3a18-43da-86bd-dc097835d79dn%40chromium.org.

[Kyle Charbonneau]

Another +1 to enable InsertBraces in clang-format.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/f79f795d-3a18-43da-86bd-dc097835d79dn%40chromium.org.

[Joe Mason]

I think the question of whether we should do a mass-reformat is orthogonal to whether we make this particular style change. The clang-format rules have evolved over time so whether or not we make this change, it might be worthwhile to do a mass-reformat to bring base and others up to the current standard.

Let's wait for pbos's full LSC proposal and discuss the pros and cons in a separate thread.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/f79f795d-3a18-43da-86bd-dc097835d79dn%40chromium.org.

[Nico Weber]

I don't have a strong opinion on this, but when reconsidering decisions I think it's a good idea to ask "what changed since we made the decision last time?". This prevents churn.

So, what's changed since the last time we made the decision?

I can think of a few things that changed that make this _less_ desirable in the past: Absence of braces lead to bugs like the famous "goto fail": https://www.imperialviolet.org/2014/02/22/applebug.html We've since mitigated this type of bug in not just one but two ways: We now build with both -Wunreachable-code and -Wmisleading-indentation.

One thing that has maybe changed is that we rely on clang-format more. But getting the benefit of not having to worry about braces with clang-format could also be achieved by teaching clang-format to insert braces only for ifs with bodies longer than 1 line.

So I'm fairly lukewarm about the proposal.

Like others on this thread I'm strongly against rewriting the codebase for this. I don't see any benefit in that, and it's a ton of churn.

On Mon, Nov 21, 2022 at 3:29 PM 'Peter Boström' via cxx <c...@chromium.org> wrote:

Hi folks,

I'd like to start a discussion on changing our brace policy for single-line ifs, and I assume this discussion is brought up every N years but I'd like to bring it up again. Specifically I'd like to transform:

if (foo)

  bar();

into:

if (foo) {

  bar();
}

Personally I'm a fan of the single-line ifs omitting braces, but I'm a bigger fan of code style that's enforceable by clang-format that has a shot at being globally consistent. Just like I don't want to think about indentation, I also want to stop considering whether the current file uses if (foo) bar(); or if (foo) { bar(); } as we currently permit both but should be locally consistent.

The style guide recommends always using braces, but does allow an exception for historical reasons which Chromium does make use of: "For historical reasons, we allow one exception to the above rules: if an if statement has no else or else if clauses, then the curly braces for the controlled statement or the line breaks inside the curly braces may be omitted if as a result the entire if statement appears on either a single line (in which case there is a space between the closing parenthesis and the controlled statement) or on two lines (in which case there is a line break after the closing parenthesis and there are no braces). For example, the following forms are allowed under this exception." and we are explicitly allowed to not make use of this exception: "Some projects require curly braces always."

I would like for us to stop making use of this exception, and update the Chromium .clang-format style to include the "InsertBraces" option (and also upstream it to the clang-format "Chromium" style). To try this out, I applied it to //base here. Patchset 1 is just applying clang-format without the .clang-format change to include InsertBraces so that the diff only concerns InsertBraces.

Applying clang-format to //base caused the following build errors:

* One macro instance in windows_types.h got a redefinition warning because {0} got changed to { 0 } or something like that.

* Reordering windows.h caused compile errors

* Formatting check_unittest.h breaks because __LINE__ changes for a macro invocation.

None of these errors look related to InsertBraces and reverting these changes caused the build to pass trybots. I believe this means that it's feasible to apply InsertBraces at scale. I'm happy splitting out the style decision from whether to actually apply clang-format retroactively, but if reformatting is a prerequisite for this change I'd be happy to own this transform.

One additional benefit from InsertBraces is that it's also able to fix existing style violations which I've spotted in multiple files (if-else without brackets): https://chromium-review.googlesource.com/c/chromium/src/+/4031901/5/base/allocator/partition_allocator/page_allocator.cc

Hopefully this means we can paint our bikeshed red, even if you'd prefer for it to be blue, because a machine can paint and keep it red for us in the future, so we can focus on the bikeshed interior. WDYT?

Best,

Peter

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sHfmFDrNt4UyGT5ZTGNa9WhhifmfVhdgLpxYiE2br3ukA%40mail.gmail.com.

[dan...@chromium.org]

On Tue, Nov 29, 2022 at 12:37 PM Nico Weber <tha...@chromium.org> wrote:

I don't have a strong opinion on this, but when reconsidering decisions I think it's a good idea to ask "what changed since we made the decision last time?". This prevents churn.

So, what's changed since the last time we made the decision?

I can think of a few things that changed that make this _less_ desirable in the past: Absence of braces lead to bugs like the famous "goto fail": https://www.imperialviolet.org/2014/02/22/applebug.html We've since mitigated this type of bug in not just one but two ways: We now build with both -Wunreachable-code and -Wmisleading-indentation.

One thing that has maybe changed is that we rely on clang-format more. But getting the benefit of not having to worry about braces with clang-format could also be achieved by teaching clang-format to insert braces only for ifs with bodies longer than 1 line.

This is a good framework, I agree these things have changed.

On benefit: There's additional benefit to developers to avoid having to insert {} when adding debugging or changing code - which all happens far from the clang-format loop which is part of presubmit generally. And if you clang-format locally, then -Wmisleading-indentation won't help you since the indentation is gone. Overall braces simplify the job of our tools, and of our developers.

I will also mention what hasn't changed: discussions wasting time on code review about braces.

 

So I'm fairly lukewarm about the proposal.

I don't really want to use braces myself, but I think it will be better for the project to just set a consistent rule (consistent = always do this, rather than consistent = here's a decision tree + local consistency).

Like others on this thread I'm strongly against rewriting the codebase for this. I don't see any benefit in that, and it's a ton of churn.

A big rewrite would be a large amount of churn in 1 CL. To not do the big rewrite will be a small amount of churn in a large number of CLs - probably continuing long past our own involvement in this project even.

Personal take: I think we should get better at blame skipping these types of stuff, and avoid git blame blocking us from bringing consistency or improvements to our codebase. The latter feels like a big failure to me.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAMGbLiEKQLSQrji5NkHaC%3DXhxjUrqKqytX-283ikeJxwd%2BacRw%40mail.gmail.com.

[Peter Kasting]

On Tue, Nov 29, 2022 at 9:37 AM Nico Weber <tha...@chromium.org> wrote:

So, what's changed since the last time we made the decision?

The main thing, IMO, is that the Google style guide has taken an opinionated stance that with-braces is the one true way, and all other routes are exceptions for historical reasons only. While Chrome qualifies for those historical exceptions, the last time we had a large debate about this, the style guide was more evenhanded about it all.

getting the benefit of not having to worry about braces with clang-format could also be achieved by teaching clang-format to insert braces only for ifs with bodies longer than 1 line.

Yes, this was my recommendation. Consensus seems to have been "we want the consistency now and no one is volunteering to do this work".  :/

Like others on this thread I'm strongly against rewriting the codebase for this. I don't see any benefit in that, and it's a ton of churn.

To keep things focused, let's agree that as part of THIS debate thread, rewriting everything is out of scope; If we want to rewrite everything, that debate should be its own thread, and it should be justified by things like "we've changed a bunch of style rules" or "clang-format output has changed a lot" and the very real question of "how do we avoid wasting people's time in git blame" should be sufficiently addressed.

PK

[K. Moon]

I agree that we've converged on:

1. Let's adopt insert braces.

2. Let's not reformat everything yet.

I don't think there's been any major movement on this thread since the Thanksgiving holiday, so I'm happy to consider this decided.

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFBV18BwFNTEy_BH%3DRKXDen%2BL2EDYVtbSvWyKhv34fk81A%40mail.gmail.com.

[Peter Boström]

Hi folks, this got landed last friday as crrev.com/c/4048911 and almost immediately reverted as crrev.com/c/4076867.

While we tested InsertBraces of all of //base before trying to land this, we did not sufficiently try everyday use with git cl format. It looks like partial formatting breaks down pretty badly (breaks control flow, yikes), see more details in crbug.com/1395455. The ~2 hours of this being checked in was enough to get two bugs filed, I do not believe that the breaking formatting is a one-off.

I don't know if there's a good path towards addressing any of these issues, and I don't see that we have built consensus on "let's adopt insert braces" without a tool that can enforce it. So I think status quo right now is that we haven't changed the law of the land, and if we want to take this further we'd need to fix clang-format to deal better with partial-file formatting when InsertBraces is enabled. I don't have the expertise to do so nor any hunch on how hard that would be. I'll likely mark these bugs as available and call them blocked on tooling support that doesn't break control flow this readily.

Thanks folks,

Peter

[dan...@chromium.org]

On Thu, Dec 8, 2022 at 2:26 PM 'Peter Boström' via cxx <c...@chromium.org> wrote:

Hi folks, this got landed last friday as crrev.com/c/4048911 and almost immediately reverted as crrev.com/c/4076867.

While we tested InsertBraces of all of //base before trying to land this, we did not sufficiently try everyday use with git cl format. It looks like partial formatting breaks down pretty badly (breaks control flow, yikes), see more details in crbug.com/1395455. The ~2 hours of this being checked in was enough to get two bugs filed, I do not believe that the breaking formatting is a one-off.

I don't know if there's a good path towards addressing any of these issues, and I don't see that we have built consensus on "let's adopt insert braces" without a tool that can enforce it. So I think status quo right now is that we haven't changed the law of the land, and if we want to take this further we'd need to fix clang-format to deal better with partial-file formatting when InsertBraces is enabled. I don't have the expertise to do so nor any hunch on how hard that would be. I'll likely mark these bugs as available and call them blocked on tooling support that doesn't break control flow this readily.

.. or fully reformat everything first, right? And to do that we need to resolve whatever pain is caused by it showing up in git blame?

 

Thanks folks,

Peter

On Tue, Nov 29, 2022 at 11:40 AM K. Moon <km...@chromium.org> wrote:

I agree that we've converged on:

1. Let's adopt insert braces.

2. Let's not reformat everything yet.

I don't think there's been any major movement on this thread since the Thanksgiving holiday, so I'm happy to consider this decided.

On Tue, Nov 29, 2022 at 11:36 AM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

On Tue, Nov 29, 2022 at 9:37 AM Nico Weber <tha...@chromium.org> wrote:

So, what's changed since the last time we made the decision?

The main thing, IMO, is that the Google style guide has taken an opinionated stance that with-braces is the one true way, and all other routes are exceptions for historical reasons only. While Chrome qualifies for those historical exceptions, the last time we had a large debate about this, the style guide was more evenhanded about it all.

getting the benefit of not having to worry about braces with clang-format could also be achieved by teaching clang-format to insert braces only for ifs with bodies longer than 1 line.

Yes, this was my recommendation. Consensus seems to have been "we want the consistency now and no one is volunteering to do this work".  :/

Like others on this thread I'm strongly against rewriting the codebase for this. I don't see any benefit in that, and it's a ton of churn.

To keep things focused, let's agree that as part of THIS debate thread, rewriting everything is out of scope; If we want to rewrite everything, that debate should be its own thread, and it should be justified by things like "we've changed a bunch of style rules" or "clang-format output has changed a lot" and the very real question of "how do we avoid wasting people's time in git blame" should be sufficiently addressed.

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFBV18BwFNTEy_BH%3DRKXDen%2BL2EDYVtbSvWyKhv34fk81A%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sH6c-J%3DfrC9DyWOQjLDoC3LMCrcrED04kqPQm6HPfJ9kA%40mail.gmail.com.

[Peter Boström]

I think even with the code fully formatting we haven't proven that a partial git cl format will always do the right thing or if it will be confused by `-lines`. I'd also be worried about the footgun lying around for anyone uploading files with --bypass_hooks or whatever.

[Alexei Svitkine]

Could we detect when "partial" git cl format did something with braces and then undo it and run the full format which it sounds like it doesn't have this problem?

We presumably wrap the actual clang-format invocation behind our own script that's hooked into "git cl", so we can have logic there? (e.g. check if braces were inserted via something simple that counts # of braces before and after for each file)

-Alexei

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sGcpSGBJdLL0OSb0XZ2b%3DtjHOEUsTBPigS_RLZsinU6qw%40mail.gmail.com.

[K. Moon]

How hard would a presubmit that all if statements use braces be? That'd probably be easier to get right (even with just a regex), and then presumably there are no braces to insert incorrectly.

[David Munro]

Splitting thread to talk about the git blame aspect:

We already have a .git-blame-ignore-revs file with a bunch of cleanup commits in it, large scale changes have already happened, so as raised it'd be nice to fix it [1].

Is this just setting the local git config to use .git-blame-ignore-revs for everyone (I see a bunch of depot_tools and other config so guess this is supported) and teaching codesearch to respect the file or is there more to it?

Thanks,

Dave.

[1] I just spent time today manually skipping past cleanup CLs, so this may be close to my heart right now :P.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-4JYmxLMP17yr%3D2do2_gRZYNNY9cYqEQJWcncgzDVqSKg%40mail.gmail.com.

[Peter Kasting]

On Thu, Dec 8, 2022 at 11:53 AM Peter Boström <pb...@google.com> wrote:

I think even with the code fully formatting we haven't proven that a partial git cl format will always do the right thing

Right, this is the issue.

One of the chief arguments made in favor of "always insert braces" was "the tools can enforce it but they don't seem to properly enforce our current policy", and we didn't want to do work on clang-format. Now it seems we'll need work on clang-format for either the current policy OR the proposed policy.

Given this, I reiterate my suggestion that we fix the bugs with clang-format's support for our current policy, don't change policy, and send out an official "please do not ask authors to twiddle this beyond what clang-format does for you" email (I can write that).

PK

[Peter Kasting]

On Thu, Dec 8, 2022 at 10:09 PM David Munro <david...@google.com> wrote:

Splitting thread to talk about the git blame aspect:

We already have a .git-blame-ignore-revs file with a bunch of cleanup commits in it, large scale changes have already happened, so as raised it'd be nice to fix it [1].

Is this just setting the local git config to use .git-blame-ignore-revs for everyone (I see a bunch of depot_tools and other config so guess this is supported) and teaching codesearch to respect the file or is there more to it?

"Make local git blame work" and "make codesearch git blame work" should cover things, yes. Of course the details of how to do each of those are unknown (to me).

PK 

[K. Moon]

I agree that it's worth re-opening this discussion if clang-format requires modification either way (although if it's possible to implement a fix without changing clang-format--since it seems like full reformats do work correctly--or easier to fix InsertBraces than adding a new option, that would still lean towards InsertBraces being the easier alternative to implement).

That said, tooling support was only one part of the previous discussion. I think the points about the churn involved in adding/removing braces (including for temporary modifications) were more persuasive to me, and tooling support doesn't change that. So my own conclusion would be the same in the end.

[Peter Boström]

I think it's worth snoozing on this one a little bit.

If we're right in that control flow breaks with -lines but not otherwise that seems like a bug that upstream can consider (clang-format has enough semantic info to do the right thing, but doesn't) and we can revisit this if anything changes. I'd suspect that's a bug worth fixing to upstream regardless of our discussion.

I wouldn't want to patch any of our tools to workaround an upstream bug or always use full. Especially if not preceded by a mass rewrite but also because a clang-format roll would make unrelated changes show up in code review all the time.

Changing the rules to always require braces without tool support opens up the floodgates to nit:brace in every code review following which seems churntastic I hope we don't need to consider that.

[dan...@chromium.org]

On Fri, Dec 9, 2022 at 12:49 PM Peter Boström <pb...@google.com> wrote:

I think it's worth snoozing on this one a little bit.

If we're right in that control flow breaks with -lines but not otherwise that seems like a bug that upstream can consider (clang-format has enough semantic info to do the right thing, but doesn't) and we can revisit this if anything changes. I'd suspect that's a bug worth fixing to upstream regardless of our discussion.

I agree this should be fixed in upstream. I thought we do a lot of diff processing in git cl format, but https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/clang-format-diff.py is upstream (now?).

I wouldn't want to patch any of our tools to workaround an upstream bug or always use full. Especially if not preceded by a mass rewrite but also because a clang-format roll would make unrelated changes show up in code review all the time.

Chaotic me says that since we only roll clang-format < 1/yr we could just reformat everything at that time.

Changing the rules to always require braces without tool support opens up the floodgates to nit:brace in every code review following which seems churntastic I hope we don't need to consider that.

PRESUBMIT rules would prevent said nits too.  r"(if|while|for) \([^)]+\) *[^{]]" or something - with cross-line regex options.

[Peter Boström]

+Daniel Cheng should probably be here too. I think both hyperblame doesn't work in codesearch and blame layer is incredibly slow (so blaming through parent changes is very painful) are contributing factors here.

[dan...@chromium.org]

My personal feeling is that if blame was simply blazingly fast, the extra layers wouldn't matter as much. And then we wouldn't have to worry about "should this CL appear in blame or not" which is going to be super non-obvious for a lot of things. And stuff like refactoring, changing APIs, etc, should be in blame but is also noisy depending on what a user is trying to accomplish.

[dan...@chromium.org]

On Sat, Dec 10, 2022 at 7:40 AM <dan...@chromium.org> wrote:

My personal feeling is that if blame was simply blazingly fast, the extra layers wouldn't matter as much. And then we wouldn't have to worry about "should this CL appear in blame or not" which is going to be super non-obvious for a lot of things. And stuff like refactoring, changing APIs, etc, should be in blame but is also noisy depending on what a user is trying to accomplish.

This is to say while stuff like a whitespace change can definitely be skipped, the speed is a problem too as blame is presenting a friction for any wide change at the moment.

[K. Moon]

hans@ posted an update on https://crbug.com/1395455#c8 that indicates the issue already was fixed (or at least improved) in upstream clang-format.

[Peter Boström]

+Hans Wennborg here as well.

I did some additional testing (touching both if and else in current braceless code) and was unable to break control flow past clang-format roll. I've a reland CL up as crrev.com/c/4097043, and will holler here if it lands or we decide not to land it.

 

[David Munro]

Faster blame would be nice though that sounds like a much bigger project. FWIW, while I have memories of the blame layer in codesearch being slow I tried now and it's about as quick as loading the https://osscs.corp.google.com/ homepage (<<1 second), even jumping randomly through history. Maybe it's been improved?

Local git blame seems straightforward as long as we can set git config for Chromium, b/262328859 to see what the codesearch team have to say.

Cheers,

Dave.

[Peter Boström]

FYI this relanded as crrev.com/c/4097043, announcement sent to chromium-dev@. Revert and holler if this breaks control flow, I couldn't get it to.

[Gabriel Charette]

Landing this without mass-rewriting the codebase now results in most CLs I review having brackets added throughout entire files which really detracts from the small changes being reviewed.

Shall we mass-rewrite? If not, can we make this rule only apply to modified code rather than modified files?

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAGFX3sGYwRwnAJNYESSEk3iLjRZqReERV8MrmPmUhhdYTCEF7A%40mail.gmail.com.

[Xiaohan Wang (王消寒)]

I think "git cl format" by default only updates modified code. Some authors accidentally formatted the whole file and as a reviewer I agree with the issue raised here.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7LK1FTBi8LULzT8DTcC2WNz%2BLf6Vyw_MSO2d2PRxSHBSDQ%40mail.gmail.com.

[Greg Thompson]

yeah, since the new format is generally only applied to modified code, we end up with conditionals close together where one has braces and another doesn't. i find this visually irritating. i won't bother everyone with my opinion on the with-or-without issue, but i strongly prefer one or the other, not both.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF1j9YMnkABksjg36AWGeJxnE-ZGx5BKB444R1AwzZthnfdqkA%40mail.gmail.com.

[K. Moon]

If the formatter touches a conditional in a CL, I usually go through and fix up all the adjacent conditionals in the same function/block. That might be one way of managing the problem for now.

I do find having the braces added in a change that was going to change that line anyway to be a bit less disruptive to me, but I wouldn't object to a mass reformat, either.

[Dana Jansens]

I don't hear any dissent about doing this. Who wants to own the LSC?

[Michael Lippautz]

On the same thread people were not excited about the mass reformat if tooling (git blame, codesearch) is not able to handle it nicely. What has changed since then?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaSUH3_vAY9jB%3D2_Fh9WOJNt3JnDSe6mvXP8rbz%2BUjT%3D2g%40mail.gmail.com.

[K. Moon]

Probably the optimism that tooling could address this in a timely fashion. :-)

[Peter Kasting]

Yes, the experience getting bounced from the code search team to the global git maintainer was a bit discouraging.

That said, at least adding things to .git-blame-ignore-revs will help locally. And I think the concerns about this issue were raised by several people, but not a majority, and not necessarily as a hard blocker.

Again, I suggest getting broad feedback on this issue. Perhaps a Google survey/form instead of a discussion email thread would be quicker?

PK

[Gabriel Charette]

I wish I could own this but do not have the bandwidth, mostly wanted to raise an issue with the original outcome of this thread. IMO it's less churn to do it once then to pay for it every CL. The git-blame problem happens either way (as-is it's good citizens that get incorrectly blamed for fixing all the instances in a file, same ultimate result but spread out and inconsistent). I'd say solving this falls in the "advanced level code health CC" bucket.

A survey might help: "Do you prefer per CL churn or git blame misdirection?".

Though my experience with surveys is captured in the cartoon below and I think an "FYI, we're doing this, here's why" likely achieves the same outcome:

[Peter Kasting]

Is the LSC "clang-format the whole codebase"? If so, +1 from me to do it (but not to own it); that said, when I last proposed this, I was asked to raise on chromium-dev@ instead of just cxx@.

Note that any formatting changes should be added to the top-level .git-blame-ignore-revs file. I asked the code search team if code search could be made to respect this by default in its blame layer. They in turn asked me to request that change instead be made to `git blame` itself, so I ran it by the primary Git maintainer (who is at Google), whose reception was rather cool. I don't think it's a completely dead end, but I think "get the code search team to deal with this instead of changing git's defaults worldwide" and "just deal with it" are better uses of our time.

PK

[Daniel Cheng]

In an ideal world, we'd have neither :)

I think there is room to improve Gerrit / code search blame. It's actually mostly tolerable until renames are involved, at which point, you need to manually put together the right URL for "view blame at previous revision".

(Yes, I have filed a bug for this at b/172085397, but I'm not sure anyone is paying attention to that bug at this point)

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7LLXpO0y7DsJXdM2dcjF91rLtUS4he3daq3qbt_BnibDAA%40mail.gmail.com.

[Gabriel Charette]

Interesting, I just do it on the command-line with `git blame -L123,124 <last-blame's sha-1>~1 -- my_dir/my_file`, iterate on <last-blame's sha-1> from HEAD until you get to a good one, need to adjust -L too to match where your code of interest was prior to that revision.

Slightly tedious but I get to the bottom of it in 5 minutes tops for torny ones.

[Michael Lippautz]

On Tue, Jul 18, 2023 at 3:17 AM Daniel Cheng <dch...@google.com> wrote:

I'm not disputing it's doable; I just find it annoying especially when files have been renamed and moved over time. This is especially the case in Blink, where I have to skip past both reformattings and renames.

 

Wholehearted +1 

 

Daniel

[Dana Jansens]

File moves are much more painful than other changes. Getting back to graphics code in WebKit is the next level of fighting through renames, formatting, repository changes.. that sort of thing is not what is being proposed though.

It would be really unfortunate if code search prevented us from making working on the code and reviewing code better. There's pretty compelling reasons listed here for doing the rewrite, I think they are valid.

[Vladimir Levin]

I wanted to double check: is there going to be a survey about this or no? A lot of the time these threads grow large quickly and it's hard to keep track of who makes the decision and where

Thanks,

vmpstr

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaRjhHQ_Tjw41-7JmettxsbdKExq1no%2B_j8VL-XZTHB_Uw%40mail.gmail.com.

[Daniel Cheng]

I'm not disputing it's doable; I just find it annoying especially when files have been renamed and moved over time. This is especially the case in Blink, where I have to skip past both reformattings and renames.

Daniel

On Mon, 17 Jul 2023 at 18:09, Gabriel Charette <g...@chromium.org> wrote:

[Daniel Cheng]

I think it's fine to proceed, but I would appreciate it if other people could help +1 the bugs that have already been filed for improving blame; I don't know how much it'll help, but it might help highlight that this is not just a problem specific to dcheng. As-is, the bug hasn't seen any traction.

Daniel