clang-format style (again)

[Daniel Cheng]

I clang-format a lot of my patches, and one common complaint recently has been clang-format's behavior to collapse short function bodies into one line.

1) What's the best place to have discussion about clang-format feedback to get agreement on desired behavior? Is there (or should there be) a formal set of Chromium style owners?

2) What should clang-format do with short inline bodies? The current options are "always on a single line" or "never on a single line". One reviewer suggested having a 'strict' compliance mode where clang-format only makes changes strictly necessary to bring something into style compliance (e.g. indent, parameter alignment) but not other things (I assume examples of things that'd fall into this category are single-line ifs, etc). I don't know if the clang-format team would be willing to implement this though.

3) What's the feasibility of just using clang-format everywhere so developers/reviewers can spend less time on these sort of things? There is still code it's unable to format well, but you can now disable/enable clang-format in a block by using

  // clang-format off

  /* hard to format code here ... */

  // clang-format on

and outside of the feedback about collapsing inline bodies into one line, it seems like it's doing a pretty good job overall.

Daniel

[Nico Weber]

On Tue, Oct 28, 2014 at 4:45 PM, Daniel Cheng <dch...@chromium.org> wrote:

I clang-format a lot of my patches, and one common complaint recently has been clang-format's behavior to collapse short function bodies into one line.

1) What's the best place to have discussion about clang-format feedback to get agreement on desired behavior? Is there (or should there be) a formal set of Chromium style owners?

2) What should clang-format do with short inline bodies? The current options are "always on a single line" or "never on a single line". One reviewer suggested having a 'strict' compliance mode where clang-format only makes changes strictly necessary to bring something into style compliance

How would you detect this, without using clang-format?

 

(e.g. indent, parameter alignment) but not other things (I assume examples of things that'd fall into this category are single-line ifs, etc). I don't know if the clang-format team would be willing to implement this though.

3) What's the feasibility of just using clang-format everywhere so developers/reviewers can spend less time on these sort of things? There is still code it's unable to format well, but you can now disable/enable clang-format in a block by using

  // clang-format off

  /* hard to format code here ... */

  // clang-format on

and outside of the feedback about collapsing inline bodies into one line, it seems like it's doing a pretty good job overall.

Daniel

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Daniel Cheng]

On Tue, Oct 28, 2014 at 4:50 PM, Nico Weber <tha...@chromium.org> wrote:

On Tue, Oct 28, 2014 at 4:45 PM, Daniel Cheng <dch...@chromium.org> wrote:

I clang-format a lot of my patches, and one common complaint recently has been clang-format's behavior to collapse short function bodies into one line.

1) What's the best place to have discussion about clang-format feedback to get agreement on desired behavior? Is there (or should there be) a formal set of Chromium style owners?

2) What should clang-format do with short inline bodies? The current options are "always on a single line" or "never on a single line". One reviewer suggested having a 'strict' compliance mode where clang-format only makes changes strictly necessary to bring something into style compliance

How would you detect this, without using clang-format?

The way I imagined: it would just skip the non-strict compliance reformatting steps and leave code as it found it for those constructs.

[Maciej Pawlowski]

W dniu 2014-10-29 o 00:45, Daniel Cheng pisze:

  // clang-format off

  /* hard to format code here ... */

  // clang-format on

and outside of the feedback about collapsing inline bodies into one line, it seems like it's doing a pretty good job overall.

I'm generally opposed to cluttering code with special markup for tools one might or might not use. Instead, the tool should be smart enough to work with the code it's applied to without crutches. The tool should work for the code, not the other way around.

Daniel

-- 
BR
Maciej Pawlowski
Opera Desktop Wroclaw

[Nico Weber]

On Tue, Oct 28, 2014 at 4:45 PM, Daniel Cheng <dch...@chromium.org> wrote:

I clang-format a lot of my patches, and one common complaint recently has been clang-format's behavior to collapse short function bodies into one line.

1) What's the best place to have discussion about clang-format feedback to get agreement on desired behavior? Is there (or should there be) a formal set of Chromium style owners?

2) What should clang-format do with short inline bodies? The current options are "always on a single line" or "never on a single line".

For this question, I think this is covered by our existing rules: We try to be close to google style, and clang-format is accepted for google style. The style guide explicitly says "minimize vertical space". So for this case, I think we should accept what clang-format does. (Feel free to add me to the reviews where people push back, I'm happy to repeat this there.)

There might be other reasons where clang-format does poorly, probably best to discuss that on a case-by-case basis. If someone finds something they dislike, please use the steps at the bottom of https://code.google.com/p/chromium/wiki/ClangFormat to file a bug.

Nico

[Vincent Scheib]

On Tue, Oct 28, 2014 at 4:45 PM, Daniel Cheng <dch...@chromium.org> wrote:

We can approach this by having more portions of the code tree adopt formatting as a presubmit, currently it's only in a few places:

https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&type=cs&q=CheckPatchFormatted

[Peter Kasting]

On Tue, Oct 28, 2014 at 4:45 PM, Daniel Cheng <dch...@chromium.org> wrote:

1) What's the best place to have discussion about clang-format feedback to get agreement on desired behavior? Is there (or should there be) a formal set of Chromium style owners?

If you need to engage people about Chromium style overall, I suggest at least roping in thakis, jam, and me, as all of us have contributed meaningfully to the Chromium style guide.

2) What should clang-format do with short inline bodies? The current options are "always on a single line" or "never on a single line".

I agree with Nico's mail: there is nothing in the Chromium style guide that objects to what clang-format is currently doing (always a single line where possible), it's reasonably common in the codebase already (even pre-clang-format), and it's seemingly allowed by the Google style guide, so I think it's fine.  People should not be pushing back against it.

3) What's the feasibility of just using clang-format everywhere so developers/reviewers can spend less time on these sort of things? There is still code it's unable to format well, but you can now disable/enable clang-format in a block by using

  // clang-format off

  /* hard to format code here ... */

  // clang-format on

and outside of the feedback about collapsing inline bodies into one line, it seems like it's doing a pretty good job overall.

In general I've seen fewer pushbacks against people who've used clang-format recently.  I think its use is already fairly widespread, although it doesn't seem like we'll ever do something as drastic as clang-formatting the whole codebase.

My personal opinion of it has improved; at this point my only real axe to grind is that clang-format still formats ?: in a way that I read as violating the Chromium style guide, and never existed in the codebase before clang-format itself started introducing it everywhere; and despite (I thought) someone claiming ownership of this issue many months ago it's never been changed.  Other than that, I think clang-format's behavior and my opinions of its behavior have both converged towards agreement that it's doing OK.  I don't know what others think.

PK

[Dana Jansens]

On Wed, Oct 29, 2014 at 3:38 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Oct 28, 2014 at 4:45 PM, Daniel Cheng <dch...@chromium.org> wrote:

1) What's the best place to have discussion about clang-format feedback to get agreement on desired behavior? Is there (or should there be) a formal set of Chromium style owners?

If you need to engage people about Chromium style overall, I suggest at least roping in thakis, jam, and me, as all of us have contributed meaningfully to the Chromium style guide.

2) What should clang-format do with short inline bodies? The current options are "always on a single line" or "never on a single line".

I agree with Nico's mail: there is nothing in the Chromium style guide that objects to what clang-format is currently doing (always a single line where possible), it's reasonably common in the codebase already (even pre-clang-format), and it's seemingly allowed by the Google style guide, so I think it's fine.  People should not be pushing back against it.

3) What's the feasibility of just using clang-format everywhere so developers/reviewers can spend less time on these sort of things? There is still code it's unable to format well, but you can

I add it to PRESUBMITs where I work a lot because reviewing style is annoying, and I'd rather let a tool do it. I suggest everyone does the same.

 

now disable/enable clang-format in a block by using

  // clang-format off

  /* hard to format code here ... */

  // clang-format on

and outside of the feedback about collapsing inline bodies into one line, it seems like it's doing a pretty good job overall.

In general I've seen fewer pushbacks against people who've used clang-format recently.  I think its use is already fairly widespread, although it doesn't seem like we'll ever do something as drastic as clang-formatting the whole codebase.

My personal opinion of it has improved; at this point my only real axe to grind is that clang-format still formats ?: in a way that I read as violating the Chromium style guide, and never existed in the codebase before clang-format itself started introducing it everywhere; and despite (I thought) someone claiming ownership of this issue many months ago it's never been changed.  Other than that, I think clang-format's behavior and my opinions of its behavior have both converged towards agreement that it's doing OK.  I don't know what others think.

PK

--

[Peter Kasting]

On Wed, Oct 29, 2014 at 12:40 PM, Dana Jansens <dan...@chromium.org> wrote:

On Wed, Oct 29, 2014 at 3:38 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Oct 28, 2014 at 4:45 PM, Daniel Cheng <dch...@chromium.org> wrote:

1) What's the best place to have discussion about clang-format feedback to get agreement on desired behavior? Is there (or should there be) a formal set of Chromium style owners?

If you need to engage people about Chromium style overall, I suggest at least roping in thakis, jam, and me, as all of us have contributed meaningfully to the Chromium style guide.

2) What should clang-format do with short inline bodies? The current options are "always on a single line" or "never on a single line".

I agree with Nico's mail: there is nothing in the Chromium style guide that objects to what clang-format is currently doing (always a single line where possible), it's reasonably common in the codebase already (even pre-clang-format), and it's seemingly allowed by the Google style guide, so I think it's fine.  People should not be pushing back against it.

3) What's the feasibility of just using clang-format everywhere so developers/reviewers can spend less time on these sort of things? There is still code it's unable to format well, but you can

I add it to PRESUBMITs where I work a lot because reviewing style is annoying, and I'd rather let a tool do it. I suggest everyone does the same.

FWIW, those PRESUBMIT checks have caused problems in my Windows checkout -- e.g. getting unreadable messages about "<function at offset 0xffed55ba> needs formatting" (I'm working from memory, the actual error may have been a different unreadable string than that) -- and they're more aggressive than they really should be (as Daniel mentioned, clang-format does more than is absolutely necessary, so sometimes you'll get "should be reformatted" messages about code that's perfectly legal per the Chromium style guide just because clang-format would have picked a different way).

As a result, I have tended to ignore those sorts of warnings, which I don't think is a great outcome.  If we want to run clang-format inside a PRESUBMIT check, that's a place where I really would support some sort of "just check that the style is legal, don't complain about legal forms that aren't what clang-format would default to" behavior mode.  Having PRESUBMIT yell at me for legal code is just as annoying as getting into formatting debates on code reviews.

PK

[Dana Jansens]

On Wed, Oct 29, 2014 at 3:55 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Wed, Oct 29, 2014 at 12:40 PM, Dana Jansens <dan...@chromium.org> wrote:

On Wed, Oct 29, 2014 at 3:38 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Oct 28, 2014 at 4:45 PM, Daniel Cheng <dch...@chromium.org> wrote:

1) What's the best place to have discussion about clang-format feedback to get agreement on desired behavior? Is there (or should there be) a formal set of Chromium style owners?

If you need to engage people about Chromium style overall, I suggest at least roping in thakis, jam, and me, as all of us have contributed meaningfully to the Chromium style guide.

2) What should clang-format do with short inline bodies? The current options are "always on a single line" or "never on a single line".

I agree with Nico's mail: there is nothing in the Chromium style guide that objects to what clang-format is currently doing (always a single line where possible), it's reasonably common in the codebase already (even pre-clang-format), and it's seemingly allowed by the Google style guide, so I think it's fine.  People should not be pushing back against it.

3) What's the feasibility of just using clang-format everywhere so developers/reviewers can spend less time on these sort of things? There is still code it's unable to format well, but you can

I add it to PRESUBMITs where I work a lot because reviewing style is annoying, and I'd rather let a tool do it. I suggest everyone does the same.

FWIW, those PRESUBMIT checks have caused problems in my Windows checkout -- e.g. getting unreadable messages about "<function at offset 0xffed55ba> needs formatting" (I'm working from memory, the actual error may have been a different unreadable string than that) -- and they're more aggressive than they really should be (as Daniel mentioned, clang-format does more than is absolutely necessary, so sometimes you'll get "should be reformatted" messages about code that's perfectly legal per the Chromium style guide just because clang-format would have picked a different way).

From my POV, I don't really care if the code as-it-was was legal or not, I vastly prefer just not having to worry about it and nitpick over it as a reviewer. It's very easy for a developer to just "git cl format" if the check complains to them. In fact my upload script just does that before uploading. I'm totally ok with it reformatting otherwise legal things into different legal configurations, and when a patch comes in with unrelated whitespace changes, I'm happy to see them and they are not hard to review because I know clang-format did something right. And from that point on, the code formatting will remain stable.

[Peter Kasting]

On Wed, Oct 29, 2014 at 1:00 PM, Dana Jansens <dan...@chromium.org> wrote:

On Wed, Oct 29, 2014 at 3:55 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Wed, Oct 29, 2014 at 12:40 PM, Dana Jansens <dan...@chromium.org> wrote:

On Wed, Oct 29, 2014 at 3:38 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Oct 28, 2014 at 4:45 PM, Daniel Cheng <dch...@chromium.org> wrote:

1) What's the best place to have discussion about clang-format feedback to get agreement on desired behavior? Is there (or should there be) a formal set of Chromium style owners?

If you need to engage people about Chromium style overall, I suggest at least roping in thakis, jam, and me, as all of us have contributed meaningfully to the Chromium style guide.

2) What should clang-format do with short inline bodies? The current options are "always on a single line" or "never on a single line".

I agree with Nico's mail: there is nothing in the Chromium style guide that objects to what clang-format is currently doing (always a single line where possible), it's reasonably common in the codebase already (even pre-clang-format), and it's seemingly allowed by the Google style guide, so I think it's fine.  People should not be pushing back against it.

3) What's the feasibility of just using clang-format everywhere so developers/reviewers can spend less time on these sort of things? There is still code it's unable to format well, but you can

I add it to PRESUBMITs where I work a lot because reviewing style is annoying, and I'd rather let a tool do it. I suggest everyone does the same.

FWIW, those PRESUBMIT checks have caused problems in my Windows checkout -- e.g. getting unreadable messages about "<function at offset 0xffed55ba> needs formatting" (I'm working from memory, the actual error may have been a different unreadable string than that) -- and they're more aggressive than they really should be (as Daniel mentioned, clang-format does more than is absolutely necessary, so sometimes you'll get "should be reformatted" messages about code that's perfectly legal per the Chromium style guide just because clang-format would have picked a different way).

From my POV, I don't really care if the code as-it-was was legal or not, I vastly prefer just not having to worry about it and nitpick over it as a reviewer.

Presumably you won't have to nitpick over it as long as it's legal.  clang-format is one way to try and achieve that, but not the only way.

Indeed, if I uploaded legal code that a reviewer nitpicked because it didn't happen to match clang-format's behavior, I would be... nonplussed.

 

And from that point on, the code formatting will remain stable.

Well, except when clang-format changes its preferred formatting, which so far has tended to happen much more frequently than we change the style guide.  For example, we recently changed clang-format to remove a Chromium-specific wrapping rule it had that caused it to prefer one-arg-per-line on function calls that span multiple lines.  That change affects a lot of places in Chromium code, but both forms are technically legal.

I'm not really trying to argue against usage of clang-format so much as I'm concerned that, in its current form, it's less appropriate as a PRESUBMIT than it is as an optional tool people are encouraged to use as much as they'd like.  I would withdraw that objection if the tool could run in an "only detect formatting errors" mode.

PK

[Daniel Cheng]

​I think the Google style is actually pretty stable. I could be wrong about this (I don't have any data to back this up), but I imagine many of the wider impact clang-format formatting changes ​have been driven by our feedback to the clang-format team about the bugs/specifics of Chromium style handling. I imagine this churn should go down over time.

Daniel

[Peter Kasting]

On Wed, Oct 29, 2014 at 1:22 PM, Daniel Cheng <dch...@chromium.org> wrote:

​I think the Google style is actually pretty stable. I could be wrong about this (I don't have any data to back this up), but I imagine many of the wider impact clang-format formatting changes ​have been driven by our feedback to the clang-format team about the bugs/specifics of Chromium style handling. I imagine this churn should go down over time.

I suspect you're correct.

Also recall that I've been one of the only advocates in the past for "let's go ahead and clang-format the whole codebase", so I could certainly live in a world where we did that sort of thing.  The world in between nothing and everything is awkward, though.

Frankly, if we're really convinced clang-format is correct, I might prefer the tools just auto-clang-format my code than warning me about it in PRESUBMIT.  Dunno.

PK

[James Hawkins]

On Wed, Oct 29, 2014 at 12:38 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Oct 28, 2014 at 4:45 PM, Daniel Cheng <dch...@chromium.org> wrote:

1) What's the best place to have discussion about clang-format feedback to get agreement on desired behavior? Is there (or should there be) a formal set of Chromium style owners?

If you need to engage people about Chromium style overall, I suggest at least roping in thakis, jam, and me, as all of us have contributed meaningfully to the Chromium style guide.

2) What should clang-format do with short inline bodies? The current options are "always on a single line" or "never on a single line".

I agree with Nico's mail: there is nothing in the Chromium style guide that objects to what clang-format is currently doing (always a single line where possible), it's reasonably common in the codebase already (even pre-clang-format), and it's seemingly allowed by the Google style guide, so I think it's fine.  People should not be pushing back against it.

In Daniel's case, I am the reviewer pushing back.  The CL in question is [1].  I'm not sure my concern was fully communicated from me through to this conversation, so to recap: I have no concern with the particular style issue being discussed (pulling up short code blocks into one-liners); my issue is with the stylistic changes in this (what I would consider a) large scale change.  The CL description is "Standardize usage of virtual/override/final specifiers," but half of the files also contain unrelated style changes (pull-ups).

It sounds like the project consensus is that clang-format is the great equalizer when it comes to style, and that any changes made by it in a CL are understood to be acceptable, so I rescind my objection to the CL.

Thanks,

James

[1] https://codereview.chromium.org/684613002/

 

3) What's the feasibility of just using clang-format everywhere so developers/reviewers can spend less time on these sort of things? There is still code it's unable to format well, but you can now disable/enable clang-format in a block by using

  // clang-format off

  /* hard to format code here ... */

  // clang-format on

and outside of the feedback about collapsing inline bodies into one line, it seems like it's doing a pretty good job overall.

In general I've seen fewer pushbacks against people who've used clang-format recently.  I think its use is already fairly widespread, although it doesn't seem like we'll ever do something as drastic as clang-formatting the whole codebase.

My personal opinion of it has improved; at this point my only real axe to grind is that clang-format still formats ?: in a way that I read as violating the Chromium style guide, and never existed in the codebase before clang-format itself started introducing it everywhere; and despite (I thought) someone claiming ownership of this issue many months ago it's never been changed.  Other than that, I think clang-format's behavior and my opinions of its behavior have both converged towards agreement that it's doing OK.  I don't know what others think.

PK

--

[Adrienne Walker]

2014-10-29 12:55 GMT-07:00 Peter Kasting <pkas...@chromium.org>:
> On Wed, Oct 29, 2014 at 12:40 PM, Dana Jansens <dan...@chromium.org> wrote:

>> I add it to PRESUBMITs where I work a lot because reviewing style is
>> annoying, and I'd rather let a tool do it. I suggest everyone does the same.

Strongly seconded. A PRESUBMIT is absolutely the right place for this
check to lessen review burden.

> FWIW, those PRESUBMIT checks have caused problems in my Windows checkout --
> e.g. getting unreadable messages about "<function at offset 0xffed55ba>
> needs formatting"

Fixed in https://codereview.chromium.org/665493002 last week.

[Ben Wells]

On Thu, Oct 30, 2014 at 9:12 AM, Adrienne Walker <en...@chromium.org> wrote:

2014-10-29 12:55 GMT-07:00 Peter Kasting <pkas...@chromium.org>:
> On Wed, Oct 29, 2014 at 12:40 PM, Dana Jansens <dan...@chromium.org> wrote:

>> I add it to PRESUBMITs where I work a lot because reviewing style is
>> annoying, and I'd rather let a tool do it. I suggest everyone does the same.

Strongly seconded.  A PRESUBMIT is absolutely the right place for this
check to lessen review burden.

Strongly thirded. As scheib@ has mentioned we do this in extensions land and it has been successful. Debating obscure style questions isn't a valuable use of time, imho.

 

> FWIW, those PRESUBMIT checks have caused problems in my Windows checkout --
> e.g. getting unreadable messages about "<function at offset 0xffed55ba>
> needs formatting"

Fixed in https://codereview.chromium.org/665493002 last week.

[Peter Kasting]

On Wed, Oct 29, 2014 at 3:12 PM, Adrienne Walker <en...@chromium.org> wrote:

> FWIW, those PRESUBMIT checks have caused problems in my Windows checkout --
> e.g. getting unreadable messages about "<function at offset 0xffed55ba>
> needs formatting"

Fixed in https://codereview.chromium.org/665493002 last week.

Great, thanks!

PK

[Colin Blundell]

On Wed Oct 29 2014 at 9:28:28 PM Peter Kasting <pkas...@chromium.org> wrote:

On Wed, Oct 29, 2014 at 1:22 PM, Daniel Cheng <dch...@chromium.org> wrote:

I think the Google style is actually pretty stable. I could be wrong about this (I don't have any data to back this up), but I imagine many of the wider impact clang-format formatting changes have been driven by our feedback to the clang-format team about the bugs/specifics of Chromium style handling. I imagine this churn should go down over time.

I suspect you're correct.

Also recall that I've been one of the only advocates in the past for "let's go ahead and clang-format the whole codebase", so I could certainly live in a world where we did that sort of thing.  The world in between nothing and everything is awkward, though.

I'm an advocate for this as well, fwiw.

 

Frankly, if we're really convinced clang-format is correct, I might prefer the tools just auto-clang-format my code than warning me about it in PRESUBMIT.  Dunno.

Strong +1.