Use clang-format before submit. Discuss.

[Greg Spencer]

Hi Folks,

If you haven't checked it out yet, check out: http://clang.llvm.org/docs/ClangFormat.html 

[HUGE props to the LLVM team!]

I would like to propose that we have someone (or a couple of someones: I'd volunteer to help) run clang-format over the entire chromium codebase (in pieces, of course), test it, check it in, and from then on make it part of the presubmit script for Chromium to check and see that the code to be submitted doesn't change when run through clang-format.

The motivation is simple: eliminate the useless discussions of whether a statement fits on one line, or how many spaces go before a colon in an initializer list (and about a thousand other nits).  It would improve our productivity, make our coding style entirely consistent, and eliminate argument and nit-picking.

The risk is that clang-format will do something that changes the way the code executes.  Because we can already build with clang (and it uses the same front end), I would think that is pretty unlikely, but we can run tests to make sure that doesn't happen (and report bugs to the LLVM team).

It just seems to me that this is a no-brainer: a few engineer weeks invested in making this happen would reap engineer months back in return (think of all the times you've had to modify a CL just to fix formatting and re-compile, re-upload, and wait for LTGM!)

What say you?

-Greg.

P.S. Let the centi-thread begin. :-)

[Avi Drissman]

clang-format formats the code in _a_ way that is consistent with our style guidelines, but there are many other acceptable ways, and in several cases that I've seen, they're preferable (at least to me).

Doing this en-masse would entail a ridiculous amount of code churn, for a goal that I don't see as outweighing the cost.

Avi

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

[Rouslan Solomakhin]

From my experience, different owners will enforce different variations of Chromium coding style. What should we do if we disagree with how clang-format behaves? 

--

[Jeremy Apthorp]

I mourn for `git blame` :(

[Thiago Farina]

On Tue, Apr 30, 2013 at 8:15 PM, Rouslan Solomakhin
<rou...@chromium.org> wrote:
> From my experience, different owners will enforce different variations of
> Chromium coding style. What should we do if we disagree with how
> clang-format behaves?
>

We should teach clang-format -style=Chromium to fit our coding style IMO.

--
Thiago

[Thiago Farina]

On Tue, Apr 30, 2013 at 8:17 PM, Jeremy Apthorp <jer...@google.com> wrote:
> I mourn for `git blame` :(
>

Yes, that is downside when doing this kind of reformats :(

--
Thiago

[Ryan Sleevi]

On Tue, Apr 30, 2013 at 4:10 PM, Greg Spencer <gspe...@chromium.org> wrote:

> Hi Folks,
>
> If you haven't checked it out yet, check out:
> http://clang.llvm.org/docs/ClangFormat.html
> [HUGE props to the LLVM team!]

Is ClangFormat stable yet? There's still been regular churn about
cleaning up edge cases

For example, I thought it still had trouble with macros like OVERRIDE
and *_EXPORT (or any other attribute-modifying macro). We use these
pretty heavily, so are we sure it yields "correct" output? If it
doesn't, "how bad" is the incorrect output, and is it something we're
willing to live with?

>
> I would like to propose that we have someone (or a couple of someones: I'd
> volunteer to help) run clang-format over the entire chromium codebase (in
> pieces, of course), test it, check it in, and from then on make it part of
> the presubmit script for Chromium to check and see that the code to be
> submitted doesn't change when run through clang-format.

Going back to stability - using which version of clang-format? The
pinned version? Does this mean that as clang-format changes its layout
flow, users will have to reformat files to match the latest behaviours
and/or bugs?

This also will create a huge amount of diff churn - and perhaps
ongoing. This has been a general sore point for any format discussions
cleanup (for example, cleanups of method/typename ordering to match
what the style guide advises)

>
> The motivation is simple: eliminate the useless discussions of whether a
> statement fits on one line, or how many spaces go before a colon in an
> initializer list (and about a thousand other nits). It would improve our
> productivity, make our coding style entirely consistent, and eliminate
> argument and nit-picking.
>
> The risk is that clang-format will do something that changes the way the
> code executes. Because we can already build with clang (and it uses the
> same front end), I would think that is pretty unlikely, but we can run tests
> to make sure that doesn't happen (and report bugs to the LLVM team).

What about Windows - where clang-format can parse the AST, but we've
never run binaries with it.

>
> It just seems to me that this is a no-brainer: a few engineer weeks invested
> in making this happen would reap engineer months back in return (think of
> all the times you've had to modify a CL just to fix formatting and
> re-compile, re-upload, and wait for LTGM!)

This also seems like it can introduce real debt (the loss of blame
clarity and the ongoing chasing of the latest format whims). Are the
current costs worth it?

>
> What say you?
>
> -Greg.
>
> P.S. Let the centi-thread begin. :-)
>

[Dirk Pranke]

I think it would be a reasonably and interesting exercise to at least do the first two steps (run it over the codebase and test it) to see what happens and what the churn might look like.

In the absence of real data it'll be hard to make an informed decision.

Personally, I think it's a tremendous idea, well worth the blame hiccup, but that's just my opinion.

-- Dirk

On Tue, Apr 30, 2013 at 4:10 PM, Greg Spencer <gspe...@chromium.org> wrote:

--

[Gregg Tavares]

Could we do it only for new files and or maybe figure out a way only to do it for patches. ie, only reformat lines for which blame would change anyway? Not sure how useful that would be but maybe it's worth investigating

 

--
Thiago

[Thiago Farina]

On Tue, Apr 30, 2013 at 8:23 PM, Dirk Pranke <dpr...@chromium.org> wrote:
> I think it would be a reasonably and interesting exercise to at least do the
> first two steps (run it over the codebase and test it) to see what happens
> and what the churn might look like.
>

It'll be big. Yes, pretty big I think. I ran it one time in a couple
of files under //ui and I'm not sure I liked all the indentations it
did.


--
Thiago

[Greg Spencer]

On Tue, Apr 30, 2013 at 4:19 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

Is ClangFormat stable yet? There's still been regular churn about
cleaning up edge cases

I don't know about that yet, but the answer there is to wait until it is stable enough that we're comfortable with it.

Going back to stability - using which version of clang-format? The

pinned version? Does this mean that as clang-format changes its layout
flow, users will have to reformat files to match the latest behaviours
and/or bugs?

I say we use whichever clang-format is bundled with the version of clang that we're using on the clang-bots.

This also will create a huge amount of diff churn - and perhaps
ongoing. This has been a general sore point for any format discussions
cleanup (for example, cleanups of method/typename ordering to match
what the style guide advises)

Yes, that is a price to be paid, to be sure.  I would be willing to pay that price on a one-time basis to never have to discuss or look for formatting issues in a CL again.

The going-forward question is a good one: What do we do if clang-format changes how it formats Chromium code?  I don't know the answer to that, but my gut feeling is that we would treat it like we do any coding-style change currently: we just update files as they are touched.

What about Windows - where clang-format can parse the AST, but we've

never run binaries with it.

We have some tests written there, I think. :-)

This is a concern we'd have to evaluate.

 

This also seems like it can introduce real debt (the loss of blame

clarity and the ongoing chasing of the latest format whims). Are the
current costs worth it?

I vote yes.  I would say I correct or find at least one format-only related thing in every CL that has more than a couple of lines.  This would get back the time it takes to correct, verify (compile and test), and re-upload for every instance of that.

-Greg.

[Greg Spencer]

On Tue, Apr 30, 2013 at 4:14 PM, Avi Drissman <a...@chromium.org> wrote:

clang-format formats the code in _a_ way that is consistent with our style guidelines, but there are many other acceptable ways, and in several cases that I've seen, they're preferable (at least to me). 

 

W

ell, but I think this is missing the bigger picture.  Isn't is more important that we're consistent (and I mean mechanically consistent) across the entire codebase than it is to have your pet style nit go your way in the code you often edit, and my way in mine?

Pretty much, any large coding style issue is addressed by the coding style guide.  The only things left are really pretty darn ambiguous and "judgement calls".  We're just letting the LLVM team make those judgement calls in one consistent direction across the whole codebase.

And, if we truly don't like how it does it, we can change the style guide.  There's a specific setting in clang-format for Chromium code that we can tweak until it matches that new preference (which we would ideally do before the "big switcharoo", naturally).

-Greg.

[Thiago Farina]

On Tue, Apr 30, 2013 at 8:29 PM, Greg Spencer <gspe...@google.com> wrote:
> I vote yes. I would say I correct or find at least one format-only related
> thing in every CL that has more than a couple of lines. This would get back
> the time it takes to correct, verify (compile and test), and re-upload for
> every instance of that.
>

I think this argument is pretty good. With clang-format we would have
one TRUE single formatting style and would never have to discuss it
again in codereviews. I see people discussing when to put {} for
single/multiple line if statements, where to put OVERRIDE/const, the
":" in inheritance, one arg per line or until it fits, etc, the list
of style nits goes on...

--
Thiago

[Rachel Blum]

Very much in favor. If I cared where and on what line things were, I'd write poetry ;)

I'm a bit worried by losing git blame past that point in time, but the benefit of never having to fix style nits (or appease the presubmit gods :) outweighs this by far. It's not as if blame history _stops_ before the "big format", and we already have more than enough cases in the code base where you hunt blame across several large refactorings.

 - rachel.

[Renato de Sousa]

FWIW, I find that svn blame is almost never a one-step process anyway. When trying to track the change that introduced a behavior, more often than not I have to recurse through 3 or 4 levels of indentation changes, code moves, refactors, etc. Adding an extra step there would be annoying, but not the end of the world.

I recommend using a special "refactor" role account for that, though - to make it easier to spot places where you have to go back an extra step in blame, and so you don't get accused of using this as an underhanded way to get to the top of the leaderboard :)

On Tue, Apr 30, 2013 at 4:36 PM, Thiago Farina <tfa...@chromium.org> wrote:

[Greg Spencer]

On Tue, Apr 30, 2013 at 4:48 PM, Renato de Sousa <rms...@chromium.org> wrote:

I recommend using a special "refactor" role account for that, though - to make it easier to spot places where you have to go back an extra step in blame, and so you don't get accused of using this as an underhanded way to get to the top of the leaderboard :)

Aww, you've uncovered my evil plot!  :-)

[Yes, excellent point.]

-Greg.

[Christian Biesinger]

Sure, but this would require even more steps. I think the benefit of
this reformatting is tiny, whereas the cost of slower git blame is
big. I really don't like the proposal.

-christian

On Tue, Apr 30, 2013 at 4:48 PM, Renato de Sousa <rms...@chromium.org> wrote:

[Matt Perry]

Meta-comment: It seems every time someone suggests some large-scale code cleanup like this, one of the biggest drawbacks is that it messes up the revision history. It's a shame we can't make our tools work for us here. Maybe there's some magic that would prevent a change like this from showing up in svn/git blame.

[Dan Beam]

I, too, welcome my machine overlords.  Having used go's fmt (a magical, time-saving, nit-busting tool) I'm all for automatic formatting.

Re: {git,svn} blame - this argument is the same of every sweeping refactor -- is the one time cost worth the heaps and heaps of developer time saved?  I've never regretted writing a CSS style presubmit.  CSS reviews/code are a tiny fraction of Chrome and my checker doesn't automatically reformat the code for you -- clang-format seems like a no-brainer in comparison.

-- 

Dan Beam

db...@chromium.org

P.S. Maybe lines changed during The Great Clang Format™ could be attributed to some known user (e.g. forma...@chromium.org) so that poor humans know instantly to look further back in VCS history?

[Greg Spencer]

On Tue, Apr 30, 2013 at 4:51 PM, Christian Biesinger <cbies...@google.com> wrote:

Sure, but this would require even more steps. I think the benefit of
this reformatting is tiny, whereas the cost of slower git blame is
big. I really don't like the proposal.

Well, but it's not the readability benefit of the reformatting itself that I'm looking for here: I could really care less about that since most of the differences are nits.

What I want to get back is the hours spent finding, arguing about and fixing formatting problems, (and the time for newbies to learn it to the point where they don't get pummelled by it).  I do git blame much less often than I submit or perform a code review, and with a role account doing the one-time change, we could even theoretically make a version of git-blame that skips those automatically and jumps to the previous version to find out the "real" blame for that line or lines.

-Greg.

[Thiago Farina]

On Tue, Apr 30, 2013 at 8:56 PM, Dan Beam <db...@chromium.org> wrote:
> P.S. Maybe lines changed during The Great Clang Format™ could be attributed
> to some known user (e.g. forma...@chromium.org) so that poor humans know
> instantly to look further back in VCS history?
>

format-bot@ sounds great! :)

--
Thiago

[Christian Biesinger]

On Tue, Apr 30, 2013 at 4:57 PM, Greg Spencer <gspe...@google.com> wrote:
> What I want to get back is the hours spent finding, arguing about and fixing
> formatting problems, (and the time for newbies to learn it to the point
> where they don't get pummelled by it). I do git blame much less often than
> I submit or perform a code review, and with a role account doing the
> one-time change, we could even theoretically make a version of git-blame
> that skips those automatically and jumps to the previous version to find out
> the "real" blame for that line or lines.

Hm, I guess the difference in opinion here is because I don't recall
having had any style discussions in my patches. Are they really that
common?

-christian

[Thiago Farina]

On Tue, Apr 30, 2013 at 9:01 PM, Christian Biesinger
<cbies...@chromium.org> wrote:
> Hm, I guess the difference in opinion here is because I don't recall
> having had any style discussions in my patches. Are they really that
> common?
>

I think at least in Chromium they are. Maybe in WebKit land that is
more relax/careless?

--
Thiago

[Greg Spencer]

Or they're just better at following their style guide, (or the guide is more clear, or more lax).  :-)

-Greg.

[John Bauman]

Or their presubmit is better at finding style errors.

--

[Alec Flett]

Yes please - I'm so sick of doing extra round-trips through code reviews just because I forgot to wrap something in the right way, or whatever. Its a waste of my and my reviewers' time.

Here's an idea which I've been rolling into my own workflow manually:

Before uploading a file via git cl, try reformatting the file. If the LOC changed via reformatting is less than the lines of code you're actually changing, then reformat before uploading.

The reasoning: if you're doing significant surgery to a file, you might as well reformat other stuff in the file while you're there. If you're doing some kind of change that touches a few files with just one or two lines of code (i.e. #include change, or paramter change to an API) then you don't want to do any kind of mass-reformat those files. 

I'd love a 'git cl reformat' that followed these rules. You could even have some kind of ratio of lines changed vs lines reformatted.

Alec

[Renato de Sousa]

It's not so much the time spent arguing *after* it's sent for review, it's more the time wasted while still coding the change, looking at a function call and deciding where's the best place to split the line. Depending on your assertiveness, that can steal some precious cycles you could be using to think about the actual logic instead. I'm fine with having that choice taken away from me, in exchange for those cycles back.

Now that I think of it, another option (although I don't know how feasible it is) is to have a presubmit script that applies it only to *changed* lines (or adjacent lines if you changed a line that is split), immediately before sending a cl for review. It still gets rid of style discussions in the future, but doesn't screw with blame or require a huge refactor CL. It still might leave individual files inconsistent, but I'd argue that at this level, as long as it doesn't change indentation, the inconsistency will be largely irrelevant. I'm fine with any option, as long as I can opt into it, and out of making style decisions.

[Peter Kasting]

On Tue, Apr 30, 2013 at 4:34 PM, Greg Spencer <gspe...@google.com> wrote:

On Tue, Apr 30, 2013 at 4:14 PM, Avi Drissman <a...@chromium.org> wrote:

clang-format formats the code in _a_ way that is consistent with our style guidelines, but there are many other acceptable ways, and in several cases that I've seen, they're preferable (at least to me). 

 

W

ell, but I think this is missing the bigger picture.  Isn't is more important that we're consistent (and I mean mechanically consistent) across the entire codebase than it is to have your pet style nit go your way in the code you often edit, and my way in mine?

IMO, no.

The Google style guide intentionally does not cover a lot of things.  As a readability reviewer I'm often in a position where I either recommend (but do not require) some change the style guide doesn't cover.  It's reasonable for people to accept or reject these suggestions, and both happen.

Similarly, in Chromium, there are a lot of different conventions in different areas which are equally legal under the style guide.  These don't differ at random; they differ because different people find different things to be more readable, useful, or consistent, sometimes significantly so.

I'm uncomfortable overriding this.  I'm not convinced that the current state of the world is more costly than a world in which formatting was more consistent, but for some people on the team less readable in code that they own and work with every day.  I don't see much of a problem with your code following your pet style nit and my code following mine, whereas I have a big problem with my code _not_ following my pet style nit.  This is because I am strongly opinionated about what I find readable in ways that go well beyond what the style guide specifies.  (My opinion could not possibly be more different from Rachel's "If I cared where and on what line things were, I'd write poetry".)  Out of respect for other people who feel just as strongly but differ on what they prefer, I'm not even comfortable with a hypothetical "well, what if we made it always do what _you_ want" formatter.

I would be OK with an "unopinionated" formatter that enforces the letter of the law and doesn't change a single thing outside that -- but I suspect such a thing would be difficult to write, and of course it would not generate the complete consistency that you see as the benefit of this change.  It would, however, eliminate outright violations of the Google style guide, and at least reduce the number of cases where people need to reformat a style-related issue.

PK

[Rachel Blum]

On Tue, Apr 30, 2013 at 6:01 PM, Peter Kasting <pkas...@chromium.org> wrote:

This is because I am strongly opinionated about what I find readable in ways that go well beyond what the style guide specifies.  (My opinion could not possibly be more different from Rachel's "If I cared where and on what line things were, I'd write poetry".)

Just so we're clear, that was a bit tongue-in-cheek - I still like my code _readable_. I'm just happy with any number of readable formats. And so to me the formatter is a readability no-op with a slight acceleration on the review side. But if for a sufficiently large number of Chromites, the formatter output were _significantly_ less readable than before, things would look different.

 - rachel

[Scott Hess]

On Tue, Apr 30, 2013 at 4:57 PM, Greg Spencer <gspe...@google.com> wrote:

What I want to get back is the hours spent finding, arguing about and fixing formatting problems, (and the time for newbies to learn it to the point where they don't get pummelled by it).  I do git blame much less often than I submit or perform a code review, and with a role account doing the one-time change, we could even theoretically make a version of git-blame that skips those automatically and jumps to the previous version to find out the "real" blame for that line or lines.

There is no way to ever get back the sunk time spent on formatting problems.  You can only save that time going forward, which doesn't really require running clang-format over the entire codebase, only over things you're changing.

WRT whether you personally run git blame more or less often than you personally make code-formatting decisions, that's not really relevant.  In my experience, when someone is running git-blame it's often because they're trying to fix something which was too subtle to figure out from just looking at the code.  I think it's pretty reasonable to add some friction on the writing-new-code side in the interests of making it easier to figure out how things got to be the way they are, if only because if it's too hard to manage the latter, people will just give up (you are unlikely to simply give up on your CL just because someone wanted you to reformat a function call).

-scott

[Greg Spencer]

On Tue, Apr 30, 2013 at 6:01 PM, Peter Kasting <pkas...@chromium.org> wrote:

The Google style guide intentionally does not cover a lot of things.  As a readability reviewer I'm often in a position where I either recommend (but do not require) some change the style guide doesn't cover.  It's reasonable for people to accept or reject these suggestions, and both happen.

When I did readability reviews, I did the same thing, and when I wrote the Google style guide for ActionScript, I intentionally left things unspecified to leave them to the discretion of the coder when I felt like specifying something specific one way or the other wouldn't bring any major difference in readability.

But what I hear you saying is that as long as it doesn't contradict the style guide, you'd accept anything reasonable.  Hopefully the LLVM folks have done something reasonable with the unspecified pieces, and where they haven't I'm sure their users (us) will correct them.

So we can expect that anything that clang-format produces will at minimum conform to the style guide (and that is our Chromium style guide, not the more generic Google one), and for cases that are left out, it will produce at least nominally readable code that would be acceptable to you, and for most cases will produce the version that has passed the test of many different users and so will be the "best pick" for those users (the people who will be reading your code).

Similarly, in Chromium, there are a lot of different conventions in different areas which are equally legal under the style guide.  These don't differ at random; they differ because different people find different things to be more readable, useful, or consistent, sometimes significantly so. 

I'm uncomfortable overriding this.  I'm not convinced that the current state of the world is more costly than a world in which formatting was more consistent, but for some people on the team less readable in code that they own and work with every day.

Well, but the main point of readability is not so I can read my own code, but rather so that someone else who normally works on another part of the code can read it when they need to.  Consistency wins there, because if the code is formatted identically everywhere, then I can go into any directory and it will be readable to me.  It's the reason that the style guide addresses formatting at all.  Making your code subtly different than mine just because it's in a different part of the tree, or you have a different opinion than your neighbor is not helping me read your code.

I don't see much of a problem with your code following your pet style nit and my code following mine, whereas I have a big problem with my code _not_ following my pet style nit.  This is because I am strongly opinionated about what I find readable in ways that go well beyond what the style guide specifies.  (My opinion could not possibly be more different from Rachel's "If I cared where and on what line things were, I'd write poetry".)  Out of respect for other people who feel just as strongly but differ on what they prefer, I'm not even comfortable with a hypothetical "well, what if we made it always do what _you_ want" formatter.

OK, I hear what you're saying, but I noticed that you allowed GMail to wrap that paragraph for you.  We used to have to wrap our paragraphs ourselves, and we've mostly stopped doing that.  It might be that GMail wraps paragraphs worse than I do, but I'm not about to start doing it again because it's a waste of time and a royal PITA when I have to insert some text in the middle.  Plus, it's readable either way.

This is all I'm saying: use a tool where a tool can produce a readable result more efficiently and consistently than you can, so you can focus your code writing and reviews on the substance and let the formatting fade into the background.

I hear that you're strongly opinionated about what you find readable, but I contend that if everything were formatted the same way, you'd come to see that as the most readable form after a while.  I didn't say prettiest (although I hope it would be), but most easily parsable.  I know I was highly opinionated about formatting my code when I arrived at Google, but after an adjustment period, I found that it wasn't so much the how, but the consistency that made code readable for me.

Also, consider your own time: if you spend a lot of time formatting code to be "readable", or suggesting adjustments in code reviews to make sure it's readable, then you're wasting your time if a tool can reasonably do it for you, right?  Sure, it might not produce the formatting that you might have, but the same is true of the compiler: If I was writing assembly, it would probably come out different (and maybe prettier) than the compiler's assembly, but no way am I spending time to correct it if they are functionally the same.

 

I would be OK with an "unopinionated" formatter that enforces the letter of the law and doesn't change a single thing outside that -- but I suspect such a thing would be difficult to write, and of course it would not generate the complete consistency that you see as the benefit of this change.  It would, however, eliminate outright violations of the Google style guide, and at least reduce the number of cases where people need to reformat a style-related issue.

Yes, you're right: that would be only marginally useful as a sanity check, but wouldn't stop anyone from nitpicking things, or me having to worry about where to wrap my parameter lists to fit my reviewer's preference, which is what I see as the real value of this proposal.

In the end, this is about efficiency.  Can you read other people's code now?  If so, then I contend that the output of clang-format will be no worse, and probably better than other people's code for you.  But in the mean time nobody has to waste time worrying about formatting in all of the places where they currently do: writing the code, reviewing the code, modifying the code because of reviewer comments and then testing those modifications.  Even mechanical changes like renames become much easier.  And they have the added benefit of absolute consistency.

-Greg.

[Sergey Ulanov]

On Tue, Apr 30, 2013 at 7:53 PM, Scott Hess <sh...@chromium.org> wrote:

There is no way to ever get back the sunk time spent on formatting problems.  You can only save that time going forward, which doesn't really require running clang-format over the entire codebase, only over things you're changing.

There are many ambiguous parts in the code style and different reviewers prefer different ways to approach these ambiguities. I'm afraid that just running clang-format when uploading a change will not stop all reviewers from nitpicking on formatting details. When somebody nitpicks on indentation in my CL I want to be able to just reply that it was formatted by clang-format and thus is correct (and would prefer if we didn't waste any time on it in the first place). For this to work we need all to agree that the formatting that clang-format uses is the only right way to format code. If just some developers starts using clang-format for new CLs it won't solve the problem for anybody - there will still be disagreement about proper way to resolve ambiguities.

I think the only way clang-format can actually solve the problem with the time wasted on useless style discussions in code reviews is if it is run _automatically_ for _all_ new code. Reformatting existing code just makes it a bit easier to maintain by removing inconsistencies between old and new code.  

--

Sergey Ulanov

[Greg Spencer]

On Tue, Apr 30, 2013 at 7:53 PM, Scott Hess <sh...@chromium.org> wrote:

On Tue, Apr 30, 2013 at 4:57 PM, Greg Spencer <gspe...@google.com> wrote:

What I want to get back is the hours spent finding, arguing about and fixing formatting problems

[...]

There is no way to ever get back the sunk time spent on formatting problems.  You can only save that time going forward, which doesn't really require running clang-format over the entire codebase, only over things you're changing.

Sorry, I guess I wasn't clear there: I'm not talking about reclaiming past hours, I'm talking about on an ongoing basis I want to get back time in the day spent for those things.

But actually there is a point to running it over the whole codebase instead of doing it incrementally as we modify files:  Running it over the whole codebase "resets" the codebase to a baseline that we can then modify.  If I only do it incrementally, then when I add one line to a file, the diff includes all kinds of other formatting only changes and makes it hard to review.

And if we just modify the formatting on the lines that change, then we're fragmenting the styles further, not making them more consistent.  I suppose as time goes to infinity we will eventually reformat everything, but it converges too slowly to be useful.

WRT whether you personally run git blame more or less often than you personally make code-formatting decisions, that's not really relevant.

Yes, you're right, my experience is just one example in many.

 

In my experience, when someone is running git-blame it's often because they're trying to fix something which was too subtle to figure out from just looking at the code.  I think it's pretty reasonable to add some friction on the writing-new-code side in the interests of making it easier to figure out how things got to be the way they are, if only because if it's too hard to manage the latter, people will just give up (you are unlikely to simply give up on your CL just because someone wanted you to reformat a function call).

I agree with why someone runs git-blame, but I don't understand your point here in adding friction to writing the code: I'm just unable to parse your argument: can you try explaining again?

There are many times when I run git-blame where I have to go "oh, that was the XX rename, or the YY refactor, I'll just skip back a revision."  If it was done by a role user, then I wouldn't even have to think about whether I could skip it, and might even be able to automate that into git-blame (although I admit that might be hairy).  It's not a huge burden to someone running git-blame anyhow.  Try git gui blame or adding -w, -M or -C to blame.  It's pretty easy to jump over formatting-only changes.

-Greg.

[Greg Spencer]

On Tue, Apr 30, 2013 at 10:48 PM, Sergey Ulanov <ser...@chromium.org> wrote:

I think the only way clang-format can actually solve the problem with the time wasted on useless style discussions in code reviews is if it is run _automatically_ for _all_ new code.

This is almost exactly what I'm proposing, but instead of just forcing it to run before we upload, I'm just saying we should have a presubmit check that won't let you check it in if your code isn't identical to the output of clang-format run on your code.

The difference is largely semantic, but you can suppress the presubmit check if need be, and it also won't change your code without you inspecting/testing it.  We should definitely make it easy to manually run clang-format on all the files in a CL before upload, however.

-Greg.

[Scott Hess]

I agree that if we can automate away the skip-the-role-account bit, it doesn't matter.

Mostly I didn't mean that I think we should add friction to writing code specifically to make git-blame easier.  Rather, I'm saying that IMHO it's not worth minor gains if the cost is that git-blame is harder.  My reasoning is that decoding code evolution is already hard enough, to the point where people who are trying to do that are usually trying to do it because nothing else easier has worked.  So if you make it harder, you're making exactly the wrong thing harder.

Hmm, like if there were a change which would make code easier to read, but would make the debugger less useful, it should need to make code much easier to read, not just a little easier to read.

[I should point out that IMHO the things which make Chromium code hard to read aren't generally things like how long function calls are formatted.]

-scott

[Greg Spencer]

On Tue, Apr 30, 2013 at 11:20 PM, Scott Hess <sh...@chromium.org> wrote:

Mostly I didn't mean that I think we should add friction to writing code specifically to make git-blame easier.  Rather, I'm saying that IMHO it's not worth minor gains if the cost is that git-blame is harder.  My reasoning is that decoding code evolution is already hard enough, to the point where people who are trying to do that are usually trying to do it because nothing else easier has worked.  So if you make it harder, you're making exactly the wrong thing harder.

[Now that I can parse. :-)]

Yes, I completely agree with you that we shouldn't make spelunking any harder than it already is.  Hopefully it's possible to make the impact of a mechanical change (albeit a large one) small enough that it's not adding to the cost significantly, or at least not more than the myriad of refactorings and renames already do on a more localized basis.

-Greg.

[AndrewH]

Personally, I don't really care what coding style is used as long as it is used consistently. Consistency makes it possible for the brain to quickly establish and exploit noticeable patterns, and that is (IMO) what reading code is all about. I think we should go for it. I may not "like" where the formatter decides to put braces or spaces or whatever, but I will quickly get used to it and I will be very thankful for (as many folks have pointed out) the reduced number of needless patchsets churned out by "nit" style comments.  Consistency within a file trumps the importance of consistency throughout a codebase. Running this automatically for all new files (as others have suggested) seems like a win-win to me. The challenge is what to do with all that existing stuff. So... <head-scratch>

The [git|svn]-blame case is not impossible to solve. We could probably make clang-format spit out some kind of metadata when it does the format (i.e., a listing that maps the lines pre-format to the lines post-format, thus allowing "blame" information to be preserved), but I imagine we'd want to get a feature in git/svn that allowed us to ingest that metadata along with a "formatting-only" change, updating the "blame" in a one-off manner.  I have absolutely no idea how that would be accomplished, but I imagine it is possible. Disclaimer: IAMNAE on source control. There are probably people on this list with the ability to comment on this, don't be shy... surely the topic of reformatting support is not unknown to the source control community.

As far as comparing the logical functionality of code across the formatting barrier, it seems clear that one could simply run clang-format on the old revision (and the new one) before doing the diff. I imagine that git and svn could also be patched to allow a kind of pre-diff filter that was specifically designed to accommodate formatting operations like this. If you ran the latest clang-format on both the old and new revisions, you'd magically steamroll over the need to care about which version of clang-format was used to format the given code - you'd just see a reasonable diff that showed the non-format-related work that had been done. So that's cool.

If the tools could be made to work as above, it wouldn't matter one bit whether we made the format changes all at once or one file at a time. What WOULD matter, however, is having "pure" format commits. A commit that mixes formatting changes with actual code changes would throw a big wrench into this. Since we cannot possibly require humans to remember to do a "formatting commit" before they actually update the code, I would advocate a one-time reformat across the codebase. This at least makes it *possible* to automate the tools. Heck, one could even imagine that git (though probably not SVN) would allow us to patch in the blame-map metadata later on, so we could do the reformat now and evolve the tools as time permits. Of course, I am probably being overly-optimistic.

So, my vote:

* Yes on a presubmit hook for new source files only

* Don't mess with the existing code unless we can get some direction on tools support.

-Andrew

[Torne (Richard Coles)]

On 1 May 2013 13:22, AndrewH <andrew...@google.com> wrote:

The [git|svn]-blame case is not impossible to solve. We could probably make clang-format spit out some kind of metadata when it does the format (i.e., a listing that maps the lines pre-format to the lines post-format, thus allowing "blame" information to be preserved), but I imagine we'd want to get a feature in git/svn that allowed us to ingest that metadata along with a "formatting-only" change, updating the "blame" in a one-off manner.  I have absolutely no idea how that would be accomplished, but I imagine it is possible. Disclaimer: IAMNAE on source control. There are probably people on this list with the ability to comment on this, don't be shy... surely the topic of reformatting support is not unknown to the source control community.

There is actually a kinda hilarious option that I'm sure we wouldn't want to do, but does exist: we could run clang-format on *every revision in history* and rewrite the whole of history to be formatted correctly forever. This might take a while, and we'd probably have to drop svn first. :)

 

As far as comparing the logical functionality of code across the formatting barrier, it seems clear that one could simply run clang-format on the old revision (and the new one) before doing the diff. I imagine that git and svn could also be patched to allow a kind of pre-diff filter that was specifically designed to accommodate formatting operations like this. If you ran the latest clang-format on both the old and new revisions, you'd magically steamroll over the need to care about which version of clang-format was used to format the given code - you'd just see a reasonable diff that showed the non-format-related work that had been done. So that's cool.

Git actually supports diff filters that would handle this with no other work required; see "man gitattributes" and look for the filter option. No need to patch it.

Note: I'm not actually advocating for/against doing the formatting here, just commenting on git capabilities :)

--
Torne (Richard Coles)
to...@google.com

[Greg Spencer]

On Wed, May 1, 2013 at 5:53 AM, Torne (Richard Coles) <to...@google.com> wrote:

There is actually a kinda hilarious option that I'm sure we wouldn't want to do, but does exist: we could run clang-format on *every revision in history* and rewrite the whole of history to be formatted correctly forever. This might take a while, and we'd probably have to drop svn first. :)

Haha!  Oh, that is bold.  Well, I think we could do that, but we could actually even improve the usefulness of git blame without ever changing any code if we could add that diff pass to git blame that would normalize formatting in all the revisions of the file you're looking at locally before doing the diffs, as Andrew proposed.  That would eliminate all existing format-only line changes from the history before doing the blame without actually changing any code, making the logic changes more prominent.  We should make that possible even if we don't reformat all the current code, IMO.

Git actually supports diff filters that would handle this with no other work required; see "man gitattributes" and look for the filter option. No need to patch it.

Sweet!

-Greg.

[Torne (Richard Coles)]

On 1 May 2013 14:59, Greg Spencer <gspe...@chromium.org> wrote:

On Wed, May 1, 2013 at 5:53 AM, Torne (Richard Coles) <to...@google.com> wrote:

There is actually a kinda hilarious option that I'm sure we wouldn't want to do, but does exist: we could run clang-format on *every revision in history* and rewrite the whole of history to be formatted correctly forever. This might take a while, and we'd probably have to drop svn first. :)

Haha!  Oh, that is bold.  Well, I think we could do that, but we could actually even improve the usefulness of git blame without ever changing any code if we could add that diff pass to git blame that would normalize formatting in all the revisions of the file you're looking at locally before doing the diffs, as Andrew proposed.  That would eliminate all existing format-only line changes from the history before doing the blame without actually changing any code, making the logic changes more prominent.  We should make that possible even if we don't reformat all the current code, IMO.

I think that's not currently possible in git blame; I'm not sure how blame is actually implemented, so I have no idea how difficult this would be to support :) 

 

Git actually supports diff filters that would handle this with no other work required; see "man gitattributes" and look for the filter option. No need to patch it.

Sweet!

-Greg.

[Stephen White]

On Tue, Apr 30, 2013 at 8:38 PM, Renato de Sousa <rms...@chromium.org> wrote:

Now that I think of it, another option (although I don't know how feasible it is) is to have a presubmit script that applies it only to *changed* lines (or adjacent lines if you changed a line that is split), immediately before sending a cl for review. It still gets rid of style discussions in the future, but doesn't screw with blame or require a huge refactor CL. It still might leave individual files inconsistent, but I'd argue that at this level, as long as it doesn't change indentation, the inconsistency will be largely irrelevant. I'm fine with any option, as long as I can opt into it, and out of making style decisions.

It appears that clang-format does support such an option:

"The python script clang/tools/clang-format-diff.py parses the output of a unified diff and reformats all contained lines with clang-format. [...] So to reformat all the lines in the latest git commit, just do:

git diff -U0 HEAD^ | clang-format-diff.py"

I just tried it as follows:

git diff -U0 master | /usr/lib/clang-format/clang-format-diff.py -style Chromium

and it seemed to work, at least for a trivial example. Theoretically that could be incorporated into git cl somehow.

Stephen

On Tue, Apr 30, 2013 at 4:57 PM, Greg Spencer <gspe...@google.com> wrote:

On Tue, Apr 30, 2013 at 4:51 PM, Christian Biesinger <cbies...@google.com> wrote:

Sure, but this would require even more steps. I think the benefit of
this reformatting is tiny, whereas the cost of slower git blame is
big. I really don't like the proposal.

Well, but it's not the readability benefit of the reformatting itself that I'm looking for here: I could really care less about that since most of the differences are nits.

What I want to get back is the hours spent finding, arguing about and fixing formatting problems, (and the time for newbies to learn it to the point where they don't get pummelled by it).  I do git blame much less often than I submit or perform a code review, and with a role account doing the one-time change, we could even theoretically make a version of git-blame that skips those automatically and jumps to the previous version to find out the "real" blame for that line or lines.

-Greg.

[Thiago Farina]

I don't think clang-format is ready yet:

Take this example:

From ui/base/accelerators/accelerator.cc:

bool Accelerator::IsAltDown() const {
return (modifiers_ & EF_ALT_DOWN) != 0;
}

Running "clang-format -i -style Chromium
ui/base/accelerators/accelerator.cc" produces:

bool Accelerator::IsAltDown() const { return (modifiers_ & EF_ALT_DOWN) != 0; }

Which isn't what we (most of us I think) would expect/write normally.

Another example from the same file:

Accelerator::Accelerator(KeyboardCode keycode, int modifiers)
: key_code_(keycode),
type_(ui::ET_KEY_PRESSED),
modifiers_(modifiers) {
}

We normally wrap each initialization in its own line, so it's easier
to read, but clang-format produces:
: key_code_(keycode), type_(ui::ET_KEY_PRESSED), modifiers_(modifiers) {}

So, no, I don't think clang-format will produce code easier to read,
at least not in its current shape.

--
Thiago

[Adrienne Walker]

Both of those examples are legal Chromium style.  If you want to argue that clang-format is doing the wrong thing here, you should first change the style guide.

-enne

2013/5/1 Thiago Farina <tfa...@chromium.org>

[Avi Drissman]

But that's my argument. The style guide was (and is) a guide to formatting. clang-format is conformant to the style guide, but there are many valid choices that are not the ones that it's making, and in quite a few cases it's significantly uglier or less easy to read than the others.

Avi

[Peter Kasting]

On Tue, Apr 30, 2013 at 10:21 PM, Greg Spencer <gspe...@google.com> wrote:

I hear that you're strongly opinionated about what you find readable, but I contend that if everything were formatted the same way, you'd come to see that as the most readable form after a while.

Not really.  There are a lot of pieces of Chrome that have been formatted in particular ways that they've had for a long time and yet I despise utterly and which bug me every single time I see them.

I will never like the way that John uses "using content::XYZ" for every symbol that comes from the content module, or the way some people put braces on conditionals with one-line bodies but multi-line tests.  Those sorts of things are common in Chrome code but bother me every time.

Also, consider your own time: if you spend a lot of time formatting code to be "readable", or suggesting adjustments in code reviews to make sure it's readable, then you're wasting your time if a tool can reasonably do it for you, right?

I actively enjoy reformatting my code until it feels perfect.  This probably sounds perverse to most other people.

 

In the end, this is about efficiency.  Can you read other people's code now?

Not without annoyance.

PK

[Anthony Berent]

I think this shows a problem; it makes some sense to reformat code that doesn't meet the coding standard, but one often has good reasons for making formatting choices within the coding standard. For example, I, at least, often add a blank line between semantically distinct sections of code. This is not something that can easily be explained to a formatting tool. If clang_formatter is going to reformat everything then it will make code much less readable than code formatted by somebody who understands it.

[Peter Kasting]

On Wed, May 1, 2013 at 9:24 AM, Adrienne Walker <en...@chromium.org> wrote:

Both of those examples are legal Chromium style.  If you want to argue that clang-format is doing the wrong thing here, you should first change the style guide.

No, they're not, per my reading of the Google style guide.

The constructor list section at http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constructor_Initializer_Lists#Constructor_Initializer_Lists allows initializers all on one line only when that one line is also the line containing the rest of the function prototype.  

The function definition section at http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Function_Declarations_and_Definitions#Function_Declarations_and_Definitions gives no example of a function body all on one line.  Examples in the style guide are normative.

The problem is that there are ten thousand issues like this where not everyone is coming at the problem with the same reading or style guide interpretation.  Coming to agreement on these and encoding them in the formatter is going to take years.

Also, Anthony's comment just now on adding an extra blank line between semantically distinct pieces of a file is also something I do and is an excellent example of something entirely uncovered by the style guide, which basically just says "do not use excessive vertical whitespace".

PK

[Adrienne Walker]

2013/5/1 Peter Kasting <pkas...@chromium.org>

On Wed, May 1, 2013 at 9:24 AM, Adrienne Walker <en...@chromium.org> wrote:

Both of those examples are legal Chromium style.  If you want to argue that clang-format is doing the wrong thing here, you should first change the style guide.

No, they're not, per my reading of the Google style guide.

The constructor list section at http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constructor_Initializer_Lists#Constructor_Initializer_Lists allows initializers all on one line only when that one line is also the line containing the rest of the function prototype.  

The "containing the rest of the function prototype" part of your sentence is not in the style guide.  The style guide says "Constructor initializer lists can be all on one line".  The *initializer list* is all on one line.

 

The function definition section at http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Function_Declarations_and_Definitions#Function_Declarations_and_Definitions gives no example of a function body all on one line.  Examples in the style guide are normative.

void Circle::Rotate(double /*radians*/) {}

 

The problem is that there are ten thousand issues like this where not everyone is coming at the problem with the same reading or style guide interpretation.  Coming to agreement on these and encoding them in the formatter is going to take years.

 

This is why having an automated formatter is a big win.  Nobody has to argue about these issues.  The formatter is the definitive way to format something and there are no local style rules so writing code in any part of the codebase has less friction.

2013/5/1 Avi Drissman <a...@chromium.org>

But that's my argument. The style guide was (and is) a guide to formatting. clang-format is conformant to the style guide, but there are many valid choices that are not the ones that it's making, and in quite a few cases it's significantly uglier or less easy to read than the others.

If the way code is formatted is significantly uglier and is actively harmful to readability, then it should go in the style guide, in my opinion.  If it is not significantly uglier, then that sounds more like personal preference and I'm not sure that's a strong argument against using an automated formatter.

-enne

[Avi Drissman]

On Wed, May 1, 2013 at 12:53 PM, Adrienne Walker <en...@chromium.org> wrote:

I'm not sure that's a strong argument against using an automated formatter.

What? Using automated formatters is good. Requiring their output here and disallowing human formatting is what I disagree with.

And altering the style guidelines so that we have leverage to force the formatter to generate less ugly code is ridiculous, putting the cart before the horse.

Avi

[Dominic Mazzoni]

On Wed, May 1, 2013 at 9:39 AM, Anthony Berent <abe...@chromium.org> wrote:

I think this shows a problem; it makes some sense to reformat code that doesn't meet the coding standard, but one often has good reasons for making formatting choices within the coding standard. For example, I, at least, often add a blank line between semantically distinct sections of code. This is not something that can easily be explained to a formatting tool. If clang_formatter is going to reformat everything then it will make code much less readable than code formatted by somebody who understands it.

Wait, would clang_formatter get rid of blank lines in the middle of functions? I could live with just about any other formatting decision, but this one would really bug me.

- Dominic

[Adrienne Walker]

2013/5/1 Dominic Mazzoni <dmaz...@chromium.org>

On Wed, May 1, 2013 at 9:39 AM, Anthony Berent <abe...@chromium.org> wrote:

I think this shows a problem; it makes some sense to reformat code that doesn't meet the coding standard, but one often has good reasons for making formatting choices within the coding standard. For example, I, at least, often add a blank line between semantically distinct sections of code. This is not something that can easily be explained to a formatting tool. If clang_formatter is going to reformat everything then it will make code much less readable than code formatted by somebody who understands it.

Wait, would clang_formatter get rid of blank lines in the middle of functions? I could live with just about any other formatting decision, but this one would really bug me.

Currently, it does not remove blank lines in the middle of functions (in my experience, having helped reformat a large part of cc/ with clang-format.)

-enne 

[Adrienne Walker]

2013/5/1 Adrienne Walker <en...@chromium.org>

I meant to say that it does not remove blank lines between two non-blank lines.  It does appear to condense multiple blank lines together.

-enne

[Rachel Blum]

Taking a step back, trying to figure out what we're exactly wanting to achieve. Here's what I got:

0) Keep Chromium in a state that achieves maximum readability for the greatest number of contributors.

1) Reduce mental capacity spent on formatting.

2) Reduce time spent round-tripping for formatting only

3) Reduce overhead on round-tripping, if we have to.

0 keeps clang-format contentious. Here are some alternate ideas for the others.

1) If you do care about a very specific style, clang-format won't help you. If you don't, clang-format is perfect. If you could format just the regions of code differing from our base code (i.e. master) when you edit, that would make this a pretty nice solution. [clang-format-region]

1a) In a perfect world, I'd be able to see all code clang-formatted if I want, yet I'd still only check in code I actively touched. There's probably some magic to do this, but I can't see an easy and friction-free path without accidents.

2)  I'd like it if I could say "this is clang-formatted" and get a get-out-of-jail-free card for style nitpicks, but given that OWNERS want (reasonably!) style to be coherent in their corner of the world, I doubt that works.

Short of that, the only approach that I can see is allowing reviewers to edit code in the review - they can enforce their preferred style easily, and it's no extra cost. Most reviewers I've worked with do the re-formatting in the comments anyways, because it's hard to get style points across. If we could restrict those re-formats to "equivalent code", that'd be even better. (And if the edited changes look identical to the original ones when both are seen through clang-format, that'd be achieved, no?) [review-edit]

3) One of the biggest annoyances for me personally is the fact that a style-only change requires new try runs. It's by far the biggest cost as I can see it. If patch sets could inherit the previous try results of they're equal under clang-format, that point would be addressed to. [tryjob-inheritance]

If all three of the above were implemented, are there any reasons to enforce clang-format anyways?

 - rachel

--

[Vincent Scheib]

+1 to not wasting time discussing formatting and using automation.

[Greg Spencer]

On Wed, May 1, 2013 at 8:58 AM, Thiago Farina <tfa...@chromium.org> wrote:

I don't think clang-format is ready yet:

[...]

We normally wrap each initialization in its own line, so it's easier
to read, but clang-format produces:
: key_code_(keycode), type_(ui::ET_KEY_PRESSED), modifiers_(modifiers) {}

So, no, I don't think clang-format will produce code easier to read,
at least not in its current shape.

First, I think we should definitely tweak clang-format until the majority is happy with the output before making it a requirement.  Let's not wait until we have consensus, however: that will take years, as Peter says.  The ten people who care vehemently about wrapping arguments and initializers and all the other minutia can then donate their spare time to arguing about it on a separate list, incrementally improve clang-format to match what they decide, and the rest of us will just take it as gospel and get lots more useful work done.

Second, I'd like to point out that both of the examples you cite are readable, and conform to the style guide.  One could argue that one is prettier than the other, but they could (and probably would) both be accepted by reviewers.  A quick grep of the (chromium non-webkit, non-third party) code shows that when we have multiple initializers, we don't wrap them about 23% of the time, so it's clearly an acceptable practice.  And, if we think that the majority is right here, we can easily fix clang-format so that they ALL are wrapped.

And I don't think we necessarily have to change the style guide to change clang-format: if the style guide is mute on something, then we're free to interpret it one way or another in clang-format, but the key ingredient is that we have a definitive, programmatic description of acceptable formatting.

-Greg.

[Greg Spencer]

On Wed, May 1, 2013 at 9:36 AM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Apr 30, 2013 at 10:21 PM, Greg Spencer <gspe...@google.com> wrote:

I actively enjoy reformatting my code until it feels perfect.  This probably sounds perverse to most other people.

No, it doesn't sound perverse: we're all detail freaks to some extent, or we wouldn't be programmers.  It just sounds like a waste of time to me if it's nominally readable already.

In the end, this is about efficiency.  Can you read other people's code now?

Not without annoyance.

Then this change will be a no-op for you: you'll still hate reading all the code. :-)

-Greg.

[Greg Billock]

Anything non-1TBS causes annoyance. (ducks) :-)

The point, I guess, being that the two goals of a style guide are to improve readability and decrease arguments. I'd say we get a good grade on the former, but a pretty poor grade on the latter. If the style guide produced optimal results, then it might be a big concern to yield a little readability to get greatly decrease arguments.

Since even our most enlightened and sensitive contributors don't feel that way, it seems to me like it'd be a big win to just obey the robot and save lots of argumentation for minimal (and reparable) readability harm.

--

[Victor Khimenko]

On Wed, May 1, 2013 at 10:40 PM, Greg Billock <gbil...@google.com> wrote:

Anything non-1TBS causes annoyance. (ducks) :-)

The point, I guess, being that the two goals of a style guide are to improve readability and decrease arguments. I'd say we get a good grade on the former, but a pretty poor grade on the latter. If the style guide produced optimal results, then it might be a big concern to yield a little readability to get greatly decrease arguments.

Since even our most enlightened and sensitive contributors don't feel that way, it seems to me like it'd be a big win to just obey the robot and save lots of argumentation for minimal (and reparable) readability harm.

Note that you can teach robot to do small readability tweaks which people tend to forget. For example "sizeof i" vs "sizeof(i)". One quirk of C/C++ is that is when you have "int i;" variable then both "sizeof(i)" and "sizeof i" are legal while "sizeof(int)" is legal and "sizeof int" is not. Consistent application for "sizeof i" instead of "sizeof(i)" can make code more readable, but this is kind of nitpicking which will make style guide to unwieldy for humans. No such problems with bots.

[tomm...@chromium.org]

I think this is a fantastic idea.

The 'git blame' could be not as bad as expected if it's run with the "-w" ignore whitespace issue. (Depending on how smart that flag is...)

[Mark Mentovai]

I think that Peter’s response is important. If I didn’t think that “+1” was trite and obnoxious, I’d say that.

The style guide intentionally leaves a lot of leeway for authors to express themselves. Writing code is not purely a mechanical exercise, it’s also an art, and involves communicating effectively both to machines and with other humans. A portion of what allows authors to be effective at doing the latter exists in this “permitted” or “tolerated” or “not prohibited” space. This applies both to mechanical formatting concerns in question here as well as more substantive issues of what code does and how it does it.

On Tue, Apr 30, 2013 at 9:01 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Apr 30, 2013 at 4:34 PM, Greg Spencer <gspe...@google.com> wrote:

On Tue, Apr 30, 2013 at 4:14 PM, Avi Drissman <a...@chromium.org> wrote:

clang-format formats the code in _a_ way that is consistent with our style guidelines, but there are many other acceptable ways, and in several cases that I've seen, they're preferable (at least to me). 

 

W

ell, but I think this is missing the bigger picture.  Isn't is more important that we're consistent (and I mean mechanically consistent) across the entire codebase than it is to have your pet style nit go your way in the code you often edit, and my way in mine?

IMO, no.

The Google style guide intentionally does not cover a lot of things.  As a readability reviewer I'm often in a position where I either recommend (but do not require) some change the style guide doesn't cover.  It's reasonable for people to accept or reject these suggestions, and both happen.

Similarly, in Chromium, there are a lot of different conventions in different areas which are equally legal under the style guide.  These don't differ at random; they differ because different people find different things to be more readable, useful, or consistent, sometimes significantly so.

I'm uncomfortable overriding this.  I'm not convinced that the current state of the world is more costly than a world in which formatting was more consistent, but for some people on the team less readable in code that they own and work with every day.  I don't see much of a problem with your code following your pet style nit and my code following mine, whereas I have a big problem with my code _not_ following my pet style nit.  This is because I am strongly opinionated about what I find readable in ways that go well beyond what the style guide specifies.  (My opinion could not possibly be more different from Rachel's "If I cared where and on what line things were, I'd write poetry".)  Out of respect for other people who feel just as strongly but differ on what they prefer, I'm not even comfortable with a hypothetical "well, what if we made it always do what _you_ want" formatter.

I would be OK with an "unopinionated" formatter that enforces the letter of the law and doesn't change a single thing outside that -- but I suspect such a thing would be difficult to write, and of course it would not generate the complete consistency that you see as the benefit of this change.  It would, however, eliminate outright violations of the Google style guide, and at least reduce the number of cases where people need to reformat a style-related issue.

PK

[John Abd-El-Malek]

For the record, I feel strongly against this. Peter and Mark expressed the reasons against this more eloquently than I would.

I think we have much more important problems to solve.

[Gregg Tavares]

On Wed, May 1, 2013 at 12:40 PM, John Abd-El-Malek <j...@chromium.org> wrote:

For the record, I feel strongly against this. Peter and Mark expressed the reasons against this more eloquently than I would.

I think we have much more important problems to solve.

I think the argument is we could more spend time solving these other important issues if auto-formatting saved us N man years of manually dealing with style nits

[Dana Jansens]

On Wed, May 1, 2013 at 3:50 PM, Gregg Tavares <gm...@google.com> wrote:

On Wed, May 1, 2013 at 12:40 PM, John Abd-El-Malek <j...@chromium.org> wrote:

For the record, I feel strongly against this. Peter and Mark expressed the reasons against this more eloquently than I would.

I think we have much more important problems to solve.

I think the argument is we could more spend time solving these other important issues if auto-formatting saved us N man years of manually dealing with style nits

I agree with this.

Having style that is perfect /for me/ is impossible in any reasonably sized project because everyone brings their own style to the table. I'd rather think less about it and not spend review time trying to find and/or argue over style nits.

Ideally I could run a reformatter on my local checkout and see the code the way I want to see it, then have it reformatted on upload/commit as per the repository's style.

[Greg Spencer]

On Wed, May 1, 2013 at 12:40 PM, John Abd-El-Malek <j...@chromium.org> wrote:

For the record, I feel strongly against this. Peter and Mark expressed the reasons against this more eloquently than I would.

I think we have much more important problems to solve.

Well, but that's exactly why I want to do this.  It's not a hard problem to solve technically now that the LLVM team has done the heavy lifting, and would free us from the (in my opinion) wasted work spent on formatting.

Besides the "git blame" issue, I haven't yet heard an argument from Peter, Mark, or anyone else that doesn't boil down to "because I like it my way", and I think that's a horrible argument.  It means that as a team we're more concerned with our formatting pet peevs and control issues than we are with efficiency and productivity.

-Greg.

[Avi Drissman]

On Wed, May 1, 2013 at 4:00 PM, Greg Spencer <gspe...@google.com> wrote:

I haven't yet heard an argument from Peter, Mark, or anyone else that doesn't boil down to "because I like it my way"

Then I would recommend reading their arguments again, because I don't see that in their arguments at all.

Avi 

[Mark Mentovai]

You seem to be assuming that there’s no information being conveyed by distinguishing between alternative code constructions within the “permitted,” “tolerated,” and “not forbidden” space. I don’t think that’s true.

You’re simplifying this to the point that you’re arguing for saving time, money, or some other resource, believing that your proposal has no cost beyond an implementation and conversion effort. I think that there is a readability cost.

You’re allowed to disagree with my analysis, but framing my position and arguments as “because I like it my way” is insulting, as is dismissing them based on that flawed interpretation instead of considering their merits.

[Greg Spencer]

OK, that was probably a little strong.  But I do think that the argument that human formatting is somehow far superior to what clang-format produces in terms of readability are pretty weak.

-Greg.

[Greg Spencer]

On Wed, May 1, 2013 at 1:13 PM, Mark Mentovai <ma...@chromium.org> wrote:

You seem to be assuming that there’s no information being conveyed by distinguishing between alternative code constructions within the “permitted,” “tolerated,” and “not forbidden” space. I don’t think that’s true.

I think there is a difference.  I just don't think it's significant to the point that we spend 10% of our time (a total SWAG, I admit) worrying about it.

You’re simplifying this to the point that you’re arguing for saving time, money, or some other resource, believing that your proposal has no cost beyond an implementation and conversion effort. I think that there is a readability cost.

I think I disagree with you that the readability cost is high.

You’re allowed to disagree with my analysis, but framing my position and arguments as “because I like it my way” is insulting, as is dismissing them based on that flawed interpretation instead of considering their merits.

Yeah, you're right about that, and I apologize.

-Greg.

[Dominic Mazzoni]

Possible compromise ideas:

* Could we consider allowing submodules (i.e. anything at the directory level) to opt-in, and have our tooling enforce it at that level?

* Could we consider making it extremely easy to "opt-out" of auto-formatting for a single file, function, or even block? That would allow us some flexibility even in cases where we  want to auto-format most of the code. It reminds me of "lint" and NOLINT annotations.

- Dominic

[Ben Goodger (Google)]

I'll be the obnoxious one to say +1 to your extremely well-stated summary re: the art and the skill of the author in communication with others. We should never let this be compromised.

In a utopian world it may be possible but I am concerned we don't live in that world. And here's where I agree with John - rather than trying to build that world for this specific issue, I'd rather us as a team work on higher priority technical debt repayment.

-Ben

[Greg Spencer]

On Wed, May 1, 2013 at 1:39 PM, Dominic Mazzoni <dmaz...@chromium.org> wrote:

Possible compromise ideas:

* Could we consider allowing submodules (i.e. anything at the directory level) to opt-in, and have our tooling enforce it at that level?

I actually think we have to do this anyhow, at least at the moment: Blink code follows a different style (WebKit style) than the rest of the code, so there's at least one directory that needs it.

* Could we consider making it extremely easy to "opt-out" of auto-formatting for a single file, function, or even block? That would allow us some flexibility even in cases where we  want to auto-format most of the code. It reminds me of "lint" and NOLINT annotations.

I'd really be against anything that requires us to annotate the code: that could only make it less readable and cluttered, and presubmit shouldn't be that invasive.  Note that we already have a way of skipping presubmit checks.  We could expand that to make it possible to just skip formatting checks.

-Greg.

[Greg Spencer]

On Wed, May 1, 2013 at 1:41 PM, Ben Goodger (Google) <b...@chromium.org> wrote:

I'll be the obnoxious one to say +1 to your extremely well-stated summary re: the art and the skill of the author in communication with others. We should never let this be compromised.

In a utopian world it may be possible but I am concerned we don't live in that world. And here's where I agree with John - rather than trying to build that world for this specific issue, I'd rather us as a team work on higher priority technical debt repayment.

I disagree that mechanical formatting clouds the communication and sucks the art out of coding: the art is in the communication provided by the logic, code organization and the comments far more than the formatting.  On the contrary, I actually think that communication is slightly improved overall by improving formatting consistency.  I also think that we would be able to do more useful work as a team, including debt repayment, if we could automate this and remove it from our process.  Those are just my opinions though.

But I guess I'm tilting at windmills here if most of the major code owners are against the idea.  This discussion wasn't for nothing though: I think that the idea to improve git blame by factoring out formatting changes between revisions still has some legs.

-Greg.

[Sergey Ulanov]

On Wed, May 1, 2013 at 1:42 PM, Greg Spencer <gspe...@google.com> wrote:

On Wed, May 1, 2013 at 1:39 PM, Dominic Mazzoni <dmaz...@chromium.org> wrote:

Possible compromise ideas:

* Could we consider allowing submodules (i.e. anything at the directory level) to opt-in, and have our tooling enforce it at that level?

I actually think we have to do this anyhow, at least at the moment: Blink code follows a different style (WebKit style) than the rest of the code, so there's at least one directory that needs it.

Per-directory opt-in sounds like a really good idea - it will allow to try auto-formatting without getting everybody to agree that it's the right solution. But I think we shouldn't allow different directories to auto-format to different styles (blink might be the only exception).

 

* Could we consider making it extremely easy to "opt-out" of auto-formatting for a single file, function, or even block? That would allow us some flexibility even in cases where we  want to auto-format most of the code. It reminds me of "lint" and NOLINT annotations.

I'd really be against anything that requires us to annotate the code: that could only make it less readable and cluttered, and presubmit shouldn't be that invasive.  Note that we already have a way of skipping presubmit checks.  We could expand that to make it possible to just skip formatting checks.

-Greg.

--

[Thiago Farina]

On Wed, May 1, 2013 at 6:10 PM, Greg Spencer <gspe...@google.com> wrote:
> On Wed, May 1, 2013 at 1:41 PM, Ben Goodger (Google) <b...@chromium.org>
> wrote:
>>
>> I'll be the obnoxious one to say +1 to your extremely well-stated summary
>> re: the art and the skill of the author in communication with others. We
>> should never let this be compromised.
>>
>> In a utopian world it may be possible but I am concerned we don't live in
>> that world. And here's where I agree with John - rather than trying to build
>> that world for this specific issue, I'd rather us as a team work on higher
>> priority technical debt repayment.
>
>
> I disagree that mechanical formatting clouds the communication and sucks the
> art out of coding: the art is in the communication provided by the logic,
> code organization and the comments far more than the formatting. On the
> contrary, I actually think that communication is slightly improved overall
> by improving formatting consistency.

I also think that the "permitted", "tolerated", "not prohibited" space
won't/shouldn't be affected by the coding format. The art space is
more about tricky/magic things than the indentation/formatting of the
overall code. There are many ways you call express your art without
worry about formatting, cc/ is an example of this (IMO).

--
Thiago

[Darin Fisher]

+1

[Peter Kasting]

On Wed, May 1, 2013 at 2:10 PM, Greg Spencer <gspe...@google.com> wrote:

This discussion wasn't for nothing though: I think that the idea to improve git blame by factoring out formatting changes between revisions still has some legs.

I also think that if we could at least make a formatting tool that can fix obvious, agreed-upon style violations, and run that over the codebase and on people's commits, it buys us something, even if that something is less than what you'd like.  These cases are a significant fraction of the style nits I cite.  For better or worse, most people don't remember the whole style guide, and while the vague and contentious parts are certainly the majority of the disagreement, the other parts are violated too :).  We have a linter today, but it doesn't find anywhere near all of these issues, and sometimes it gives false positives ("file is missing a trailing newline" when it isn't).

PK

[Darin Fisher]

Maybe this has already been suggested, but my take is that if we had a tool that you could run over your CL that would just fix-up the lines you are already touching, then perhaps people might choose to use that.  It might make them more efficient.  If most people start using such a tool, then it might be worth considering turning it on by default (with an opt-out).

-Darin

--

[Stephen White]

On Wed, May 1, 2013 at 7:43 PM, Darin Fisher <da...@chromium.org> wrote:

Maybe this has already been suggested, but my take is that if we had a tool that you could run over your CL that would just fix-up the lines you are already touching, then perhaps people might choose to use that.  It might make them more efficient.  If most people start using such a tool, then it might be worth considering turning it on by default (with an opt-out).

As I mentioned upthread, I think such a tool already exists:

git diff -U0 master | clang-format-diff.py -style Chromium

I tried it out for some simple cases, and it seems to work.

Stephen

[Greg Spencer]

On Wed, May 1, 2013 at 4:43 PM, Darin Fisher <da...@chromium.org> wrote:
>
> Maybe this has already been suggested, but my take is that if we had a tool that you could run over your CL that would just fix-up the lines you are already touching, then perhaps people might choose to use that. It might make them more efficient. If most people start using such a tool, then it might be worth considering turning it on by default (with an opt-out).

Yeah, it already exists. Here's the bash function I use to do that on Linux:

my_apply_format() {
local base_branch=${1:-master}
git diff -U0 $base_branch | \
/usr/lib/clang-format/clang-format-diff.py -style Chromium
}

This will format only those lines different from master, in the
context of the entire file, and save the file back in place. It does
help, as do the editor integrations that clang-format provides.

-Greg.

[Greg Spencer]

Yeah, that would be really nice. I think that it's probably hard problem though: we would need to identify what parts of the formatting code are correcting pure style guide violations, and which aren't, and I think that in itself is probably pretty ambiguous at times.

-Greg.

[Rachel Blum]

Replace the base_branch line with

local base_branch=$(git for-each-ref --format='%(upstream:short)' $(git symbolic-ref -q HEAD))

and life is even better ;)

 - rachel

[Greg Spencer]

Nice! Thanks!

Don't forget the arg $1 override:

local base_branch=${1:-$(git for-each-ref --format='%(upstream:short)'
$(git symbolic-ref -q HEAD))}

-Greg.

[Darin Fisher]

Then it sounds like we are all set for now :-)

I'm certainly going to give it a try.  Others could too.  I don't think we need to make it run by default at this point.

Perhaps it would be good to wrap this as a script in depot_tools and do something to help people discover it.

-Darin

[Chris Hopman]

This is simpler (I think it's the same, but git is crazy):

git diff @{u}

And this might be better:

git diff $(git merge-base HEAD @{u})

The first one will be wrong (for some definition of "wrong") if you have not merged/rebased after updating the tracked branch. The second one diffs against the last common commit.

-Greg.

[Torne (Richard Coles)]

On 2 May 2013 02:00, Chris Hopman <cjho...@chromium.org> wrote:

This is simpler (I think it's the same, but git is crazy):

git diff @{u}

And this might be better:

git diff $(git merge-base HEAD @{u})

Shorter still: git diff @{u}...

[Greg Spencer]

On Thu, May 2, 2013 at 3:14 AM, Miguel Garcia <mig...@google.com> wrote:

A suggestion, skip java files out of the diff for formatting purposes
since the formatting does not play well with those.

clang-format-diff.py

already filters everything except .cc, .cpp and .h files out of the diff, so no worries there.  (Except I wonder if it should also allow .cxx and .C, etc. but I guess that doesn't matter at Google).

-Greg.

[Chris Bentzel]

I've run emacs in batch mode with google-c-style.el against files
being changed and that has worked well. Sounds like the clang-based
approach is even better.

[Aaron Gable]

PSA: 'git cl format'

https://codereview.chromium.org/14629012/ introduces a new command: 'git cl format'. If you develop Chromium using git, and use proper feature branches (i.e. branches explicitly tracking an upstream, usually master or origin/master), then you can use 'git cl format' to automatically pass your current diff through clang-format. In addition, you can use 'git cl format --full' to pass every .h, .cc, and .cpp file you've touched in the current CL through clang-format.

This tool is provided for developers who want to use clang-format on their own CLs. Use at your own discretion :)

Aaron

[Zachary Turner]

As far as git blame is concerned, isn't this what git blame -S is for?  We could provide a command alias for it, so you could call it as git cblame or something, and that would auto-resolve to git blame -S with a rev-list that excludes the pretty print.

Disclaimer: I haven't ever used git blame -S, just noticed it in the options.

[Aaron Boodman]

I think you mean `git log -S`. It's awesome, way more useful than the traditional blame command.

[Gabriel Charette]

On Mon, May 6, 2013 at 3:23 PM, Aaron Gable <aga...@chromium.org> wrote:

PSA: 'git cl format'

https://codereview.chromium.org/14629012/ introduces a new command: 'git cl format'. If you develop Chromium using git, and use proper feature branches (i.e. branches explicitly tracking an upstream, usually master or origin/master), then you can use 'git cl format' to automatically pass your current diff through clang-format. In addition, you can use 'git cl format --full' to pass every .h, .cc, and .cpp file you've touched in the current CL through clang-format.

This tool is provided for developers who want to use clang-format on their own CLs. Use at your own discretion :)

I was extremely excited about this, 'till I realized it didn't work on Windows :(.

Technically the parser module of clang which does this shouldn't be OS-specific and should work on Windows right? Is it just a matter of adding it to depot_tools instead of assuming it's in /usr/lib/clang-format?

Thanks!

Gab

[Kim Gräsman]

On Fri, May 17, 2013 at 12:07 AM, Gabriel Charette <g...@chromium.org> wrote:
>
> I was extremely excited about this, 'till I realized it didn't work on
> Windows :(.
>
> Technically the parser module of clang which does this shouldn't be
> OS-specific and should work on Windows right? Is it just a matter of adding
> it to depot_tools instead of assuming it's in /usr/lib/clang-format?

As an ex-Chromium data point, I've run clang-format on Windows without hiccups.

- Kim