Reformatting blink with `clang-format -style=Chromium` (was [blink-dev] clang-format broken for blink style?)

[Nico Weber]

Ok, after 1 day we have:

* 13 in favor of reformatting with -style=Chromium (without tweaks), about half of them surprisingly strongly ("why is this even be discussed?" "in favor of reformatting to match Chromium asap" etc)

* 1 against reformatting in general because of tooling (who might feel better after dborowitz' mail about making blame work better)

* esprehn, dglazkov and 1 more against it

I think this very convincingly disproves the "silent majority is against reformatting" and shows that in fact the opposite is true.

There were some concerns about implementing this carefully (code generators need to be changed; we need to make sure all reformatted code looks ok; we need to do this in a way that makes it easy to rebase patches), but it very much sounds like the time to do this is now.

esprehn, dglazkov, I hope this changes your mind?

(changing subject to make sure everyone on blink-dev sees it)

Nico

On Mon, Nov 16, 2015 at 10:54 AM, Nico Weber <tha...@chromium.org> wrote:

Are there people other than dglazkov and esprehn opposed to this? Or in favor of this? Please reply to me privately; I'd like to get a rough overview of how major the alleged silent minority is here :-)

On Thu, Nov 12, 2015 at 7:41 AM, Dimitri Glazkov <dgla...@chromium.org> wrote:

I don't think we should be doing this right now.

:DG<

On Thu, Nov 12, 2015 at 2:18 AM, TAMURA, Kent <tk...@chromium.org> wrote:

I made an experimental CL to apply Chromium style clang-format to third_party/WebKIt/Source/wtf.

https://codereview.chromium.org/1436153002

It worked well.

Does anyone object to proceeding this?

On Mon, Nov 9, 2015 at 11:48 AM, TAMURA, Kent <tk...@chromium.org> wrote:

IMO, we may apply |clang-format --style=Chromium| without 80 column limit now.  Less difference is better obviously, and whitespace is not important.

--

TAMURA Kent
Software Engineer, Google

--

TAMURA Kent
Software Engineer, Google

[Dimitri Glazkov]

Nico,

I am glad you're passionate about this and I certainly don't want to be the "one who prevents things". 

To add a bit more structure to this discussion, I thereby anoint Jeffrey (jyasskin@) as Blink Code Style owner, and make it his responsibility to reason about the Blink style as a whole, its future state, the path toward it, and the power to make the relevant decisions along the way.

If you would like to participate along, please reach out to him separately. I am sure he'll be happy to listen.

:DG<

[Nico Weber]

On Tue, Nov 17, 2015 at 2:30 PM, Dimitri Glazkov <dgla...@chromium.org> wrote:

Nico,

I am glad you're passionate about this and I certainly don't want to be the "one who prevents things". 

To add a bit more structure to this discussion, I thereby anoint Jeffrey (jyasskin@) as Blink Code Style owner, and make it his responsibility to reason about the Blink style as a whole, its future state, the path toward it, and the power to make the relevant decisions along the way.

My point is that the feedback pretty clearly is that there shouldn't be such a thing as a Blink Code Style owner, at least for whitespace formatting.

[Dimitri Glazkov]

On Tue, Nov 17, 2015 at 2:33 PM, Nico Weber <tha...@chromium.org> wrote:

On Tue, Nov 17, 2015 at 2:30 PM, Dimitri Glazkov <dgla...@chromium.org> wrote:

Nico,

I am glad you're passionate about this and I certainly don't want to be the "one who prevents things". 

To add a bit more structure to this discussion, I thereby anoint Jeffrey (jyasskin@) as Blink Code Style owner, and make it his responsibility to reason about the Blink style as a whole, its future state, the path toward it, and the power to make the relevant decisions along the way.

My point is that the feedback pretty clearly is that there shouldn't be such a thing as a Blink Code Style owner, at least for whitespace formatting.

This might be true in the future. We're not there yet. This should be a project decision, not a vote or secret cabal-like decision, so Jeffrey is now the owner of this from the Blink side, and we surely all agree that he will come to a reasonable result with you -- whatever that'll be.

:DG<

[Jeffrey Yasskin]

I'm inclined to accept Kent's CL to reformat Blink to match
Chromium's-whitespace-style-aside-from-the-80-column-limit:
https://codereview.chromium.org/1436153002

If you see specific things you hate about the CL, please mail me in
the next 24 hours and complain about them. Otherwise, matching
Chromium seems like a win.

I'd also appreciate seeing what roadblocks we have for adopting the
rest of Chromium's style, so I've made this form to collect thoughts:
https://goo.gl/forms/ChnS89os70.

Hopefully we can converge sooner rather than later.

Jeffrey

On Tue, Nov 17, 2015 at 2:30 PM, Dimitri Glazkov <dgla...@chromium.org> wrote:

[Jeffrey Yasskin]

Given feedback so far, it looks like there's actually support for
reformatting to 80 columns, in addition to the rest of tkent's change.
If you're either overjoyed or shocked and appalled, speak up, ideally
via https://goo.gl/forms/ChnS89os70.

The patch mis-formats a few macro uses. I weakly prefer getting the
reformatting over with quickly even if it causes a small mess, and
letting folks clean up afterwards. Holler if you want me deposed now.
:)

Mad-with-power-ly yours,
Jeffrey

[Peter Kasting]

On Tue, Nov 17, 2015 at 7:53 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

The patch mis-formats a few macro uses. I weakly prefer getting the
reformatting over with quickly even if it causes a small mess, and
letting folks clean up afterwards.

It might be worth at least checking to see if the cases in question would be feasible to fix in clang-format before throwing the switch.

PK 

[Raymond Toy]

On Tue, Nov 17, 2015 at 10:56 AM, Nico Weber <tha...@chromium.org> wrote:

Ok, after 1 day we have:

* 13 in favor of reformatting with -style=Chromium (without tweaks), about half of them surprisingly strongly ("why is this even be discussed?" "in favor of reformatting to match Chromium asap" etc)

* 1 against reformatting in general because of tooling (who might feel better after dborowitz' mail about making blame work better)

* esprehn, dglazkov and 1 more against it

I think this very convincingly disproves the "silent majority is against reformatting" and shows that in fact the opposite is true.

If they responded, they're no longer silent and aren't a member of the silent majority anymore. ;-)

If this must be done, make sure it's 100% chromium.  Mostly chromium but not quite is way more confusing than just blink which is obviously different.

[Jeremy Roman]

Will this keep using Blink naming, default arg, etc. style after this, or will m_fooBar be rewritten to foo_bar_ (etc) at the same time? In subsequent changes?

[Nico Weber]

At the moment, this is only about whitespace formatting. (In part because tooling exists for that already.)

[Jeffrey Yasskin]

Yep, but do let me know what you think about the other changes, since
we'll be trying to converge on them after the initial whitespace
change.

[Elliott Sprehn]

I don't think renaming all our files to hacker_case, our inline methods, and our member variables is a productive use of time. It means changing several code generators, macros, other tooling, and cuts deeply into the usability of blame. None of this will result in shipping a better product to our users. Lets focus on product excellence: improving memory usage, performance, and fixing bugs instead. :)

If we must we can have new files follow this style, but going back and redoing all the layout/style code is not worth it.

[Peter Kasting]

On Wed, Nov 18, 2015 at 10:45 AM, Elliott Sprehn <esp...@chromium.org> wrote:

I don't think renaming all our files to hacker_case, our inline methods, and our member variables is a productive use of time. It means changing several code generators, macros, other tooling, and cuts deeply into the usability of blame. None of this will result in shipping a better product to our users. Lets focus on product excellence: improving memory usage, performance, and fixing bugs instead. :)

If we must we can have new files follow this style, but going back and redoing all the layout/style code is not worth it.

I think these things are not very different than other style changes in terms of pros, cons, and how we should approach them.  I agree that in cases where it will take massive time and effort to change existing code, it is likely better to leave it as-is and simply make new code follow new style, and change existing code as you touch it.

However, if it's possible to use automated tooling to make these changes with not too much effort, then we should make them.  "Makes blame worse" is no more or less a factor with these changes than the other style changes we're pondering.  "Won't result in a better product" is no more or less true than with these other changes.  Etc.

If anything, having different standards for file and variable names is an example of a change that throws me significantly _more_ when reading and writing Blink code than the whitespace differences do.

PK 

[Dimitri Glazkov]

On Wed, Nov 18, 2015 at 10:45 AM, Elliott Sprehn <esp...@chromium.org> wrote:

 Lets focus on product excellence: improving memory usage, performance, and fixing bugs instead. :)

Hear hear.

:DG<

[Dirk Pranke]

While I agree that we need to evaluate the cost of the proposed cleanups, of course, and I don't 

think we should change things just for change's sake, we also shouldn't act as if reformatting

code keeps us from doing the above things, and doing tasks that reduce technical debt and make it

easier for people to work on both code bases are worthwhile, too. 

-- Dirk

[Julien Chaffraix]

The explicit contract from the beginning was that once Blink was a
Chromium directory then we should start behaving as such. Switching
coding style is one of the steps for Blink developers (the minority
amoung Chromium developers) to integrate into the broader ecosystem.

Nobody will disagree with improving memory, performance and killing
bugs. The proposal is to decrease cognitive load on every developers
in the long run (which does increase productivity), at the expense of
some short-term pain.

Not that I look forward to any coding style switch or will
particularly enjoy the transition but it's a necessary evil. Going
half-way will remove most of the upsides and delaying it will not make
the endeavor easier (unless it's for some tooling improvement that
would help us go through and past the transition). I would rather stop
we stop arguing about it, embrace the future, make the painful switch
and go back to improving our code and product.

Thanks,
Julien

[Kentaro Hara]

I agree with Elliott. I think it's better to spend our efforts on more important projects. Code health does matter, but even if we work on the code health, there would be more important projects to work on (e.g., Blink Onion Soup sounds more impactful to me than fixing the coding style). The opportunity cost of fixing the coding style seems a bit too high.

--

Kentaro Hara, Tokyo, Japan

[Peter Kasting]

On Wed, Nov 18, 2015 at 3:49 PM, Kentaro Hara <har...@chromium.org> wrote:

I agree with Elliott. I think it's better to spend our efforts on more important projects. Code health does matter, but even if we work on the code health, there would be more important projects to work on (e.g., Blink Onion Soup sounds more impactful to me than fixing the coding style). The opportunity cost of fixing the coding style seems a bit too high.

Do you also disagree with the proposed whitespace reformatting?

I ask because I can't see how that's any different.  Elliott and Dimitri have argued against both, I've argued for both; those two positions I understand.  But unless you're the one other person opposed to the whitespace reformatting (or you never responded to that), then you'd be in favor of a halfway position that I don't comprehend.

PK

[Kentaro Hara]

Yeah, I'm the third person :)

The reason I'm not really happy about the whiltespace reformatting is that if we do that, we'll end up with keeping discussing & doing a lot of more work to getting Blink's coding style closer to 100% Chromium. That doesn't seem to be productive to me for its opportunity cost. The advantage of deciding to keep the current coding style is that we can avoid those discussions and spend the efforts on more impactful things.

[Peter Kasting]

On Wed, Nov 18, 2015 at 4:13 PM, Kentaro Hara <har...@chromium.org> wrote:

On Thu, Nov 19, 2015 at 9:04 AM, Peter Kasting <pkas...@google.com> wrote:

On Wed, Nov 18, 2015 at 3:49 PM, Kentaro Hara <har...@chromium.org> wrote:

I agree with Elliott. I think it's better to spend our efforts on more important projects. Code health does matter, but even if we work on the code health, there would be more important projects to work on (e.g., Blink Onion Soup sounds more impactful to me than fixing the coding style). The opportunity cost of fixing the coding style seems a bit too high.

Do you also disagree with the proposed whitespace reformatting?

I ask because I can't see how that's any different.  Elliott and Dimitri have argued against both, I've argued for both; those two positions I understand.  But unless you're the one other person opposed to the whitespace reformatting (or you never responded to that), then you'd be in favor of a halfway position that I don't comprehend.

Yeah, I'm the third person :)

OK.  I understand your position better, then :)

Let's say we end up deciding we're definitely going to make the whitespace change.  If that happens, would you prefer to leave these other aspects of the style as the existing Blink style, or do you think we'd be better-served by making a more wholesale change to Chromium style?

PK

[Dana Jansens]

I think this only works because blink only interacts with chromium through virtual interfaces that (mostly) it defines. So it doesn't have to interact with other styles. The result of onion soup is that chromium-styled classes will be used directly in blink code, and will even move into the source tree beside blink styled things. The inconsistencies are going to become more obvious and confusing with time.

I think the argument is basically do it now, or do it later and be progressively more sad about the state of things it until it happens.

If people want to work on this, unless you think it's genuinely not worth doing *ever*, I don't understand asking people to not do it right now. Maybe folks are estimating the amount of time style changes will take is much larger than what I would expect.

[Kentaro Hara]

If we go to making the whiltespace change, I'd prefer stopping there.

[Jeffrey Yasskin]

I'm trying to work out a proposal with the folks in
styleguide/c++/OWNERS, which we'll bring back to here and
chromium-dev. Folks should feel free to keep discussing here, but you
should also feel free to spend time on your day jobs, and let us waste
just 4 folks' time arguing. :)

Jeffrey

[Daniel Bratell]

Time *is* limited. I don't know how much time it will cost to convert a few thousand files, but I know that it will be be a temporary reduction in other things done for a number of factors. I also know that those changes we have in Blink that we have not been able to put in the official repository will all have to be updated, most likely manually, and that will take time. That is not an argument I'm using for or against converting style, but as an example that there are real time sinks.

I think what haraken, esprehn, dglazkov argues (and I basically agree) is that the time spent on converting style will not be regained quickly enough so we will end up with a product with slightly fewer features, slightly higher resource usage, slightly more bugs and slightly worse performance than if we don't convert style. And I apologize if I misinterpreted them.

Even if Blink is converted to the same coding style as chromium, it will not be just one coding style. There are dozens of third party projects embedded in the chromium code base and they typically have their own styles. Living with more than one coding style in a code base is the way of life in a big project and I don't think it will change any time soon.

I've responded in the form and not here but I can repeat what I wrote there. My opinion is that we should have blink and chromium styles converge (from both directions if possible) but that there is no hurry and it can be done in steps (one "component" at a time?).

/Daniel

--

/* Opera Software, Linköping, Sweden: CET (UTC+1) */

[Joel Weinberger]

I am in favor of a full Chromium style conversion, and I basically agree with Dana's view that the arguments against this seem to be pretty much arguments against *ever* doing it (which is a fine position to hold). That having been said, I personally have no objection to the "one component at a time" approach, but I would love to have a timeline and plan of what we're going to do by when (e.g. "By January 2017, we will have all code converted to Chromium style" or whatever). Otherwise I'm afraid we'll just rehash this conversation over and over again. I suppose, though, that making this sort of policy is exactly why we have Jeffrey & Co. in charge :-)

--Joel

[Joe Mason]

I find that each separate variation between blink and chromium style (spacing issues, naming issues, etc) is a separate thing to remember when switching between code bases, so changing each individual one is somewhat valuable even without going as far as having full parity. But different features have different levels of annoyance - fiddly spacing differences are hard to remember in practice, using underscores instead of CamelCase is easier because it's more obvious. And also some style rules take more work to convert / enforce automatically.

Which means that I don't think forcing a full conversion should be a goal - we should concentrate on low hanging fruit. Changes that are safe to make automatically and fix differences which slow people down significantly, sure, but don't spend a lot of effort beyond that.

Joe

[Peter Kasting]

On Thu, Nov 19, 2015 at 1:55 AM, Daniel Bratell <bra...@opera.com> wrote:

Even if Blink is converted to the same coding style as chromium, it will not be just one coding style. There are dozens of third party projects embedded in the chromium code base and they typically have their own styles. Living with more than one coding style in a code base is the way of life in a big project and I don't think it will change any time soon.

The third-party codebases that differ from Chromium's style are pretty much all things that very few developers on the team ever touch.  That's not the case with Blink.

PK 

[Dimitri Glazkov]

On Wed, Nov 18, 2015 at 5:09 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

I'm trying to work out a proposal with the folks in
styleguide/c++/OWNERS, which we'll bring back to here and
chromium-dev. Folks should feel free to keep discussing here, but you
should also feel free to spend time on your day jobs, and let us waste
just 4 folks' time arguing. :)

Just a reminder to all Blink contributors: if you haven't already, please fill out Jeffrey's form: https://goo.gl/forms/ChnS89os70

:DG<