Feature/clang format by indutny · Pull Request #3612 · nodejs/node

indutny · 2015-10-31T00:22:37Z

R=@bnoordhuis

cc @nodejs/tsc @nodejs/collaborators and everyone who cares about C++ code style in node.js

indutny · 2015-10-31T00:23:10Z

As per @bnoordhuis suggestion, we will probably need to apply this to LTS too, to make patches easier to backport.

mscdex · 2015-10-31T00:31:15Z

IMHO this (currently) makes the source code less readable.

jbergstroem · 2015-10-31T00:34:49Z

I'd like to see the option for overriding download/install by providing your own.

indutny · 2015-10-31T00:35:49Z

@jbergstroem what if you have an older clang-format version that does things differently?

indutny · 2015-10-31T00:36:21Z

@mscdex clang-format is configurable, what in particular became worse in your opinion?

Qard · 2015-10-31T00:39:02Z

I like the formatter idea in general, but the code changes this makes are rather significant. Can we start with something closer to the existing style and adapt to some "better" style incrementally? I'm not a big fan of the havoc this plays on git blame...

mikeal · 2015-10-31T00:40:44Z

@Qard my guess is that no matter how we tune it we'll end with just as big of a diff because of inconsistencies through the code. In general I think it's best to take the standard style people are used to, so either the clang defaults or perhaps the defaults used by v8 or another project we already rely on.

mscdex · 2015-10-31T00:42:11Z

@indutny The ones that really bug me right off the bat are the changes in function parameter and macro layouts.

IMHO function parameters should either fit all on one line or each parameter each on a new line.

As far as macros go, an example is the #define V(PROVIDER) change at the top of src/async-wrap.cc.

jbergstroem · 2015-10-31T00:43:07Z

Suggesting we check clang-format -version output so we can have the user provide a binary instead. As-is, this would break on our linter bots.

indutny · 2015-10-31T00:43:47Z

@mscdex the defines are limitation of clang-format, there is no way to configure it right now.

Regarding function parameters, this is how they are handled in v8. After contributing several CLs recently, I got convinced that this is actually pretty good way of doing it. Just a matter of getting used to.

Anyway, we can do some sort of voting in TSC, and I'm sure that we will account all points of view.

mikeal · 2015-10-31T00:44:50Z

This is v8's https://chromium.googlesource.com/v8/v8/+/master/.clang-format LMAO that "Google" is a builtin clang style :)

indutny · 2015-10-31T00:46:24Z

@mikeal I tried going that way, but ended up in way too many changes. I don't mind doing it, though

mikeal · 2015-10-31T00:47:11Z

@indutny if we're going to take a big ass diff anyway we might as well get to the ideal style for long-term maintenance.

trevnorris · 2015-10-31T07:42:07Z

I've worked with source that places as many arguments as possible on the same line and IMO it makes quickly scanning through code noticeably more difficult. It's not bad once you are familiar with it, but otherwise it adds a cognitive burden.

Though I assume this won't land over the weekend so I'll chime in more on Monday.

piscisaureus · 2015-10-31T07:54:28Z

Instead of reformatting the source code, what about requiring that new changes are run through git clang-format before merged. A sweeping style change like this makes a lot of things difficult (backporting, blame), and it doesn't really fix things once and for all; clang-format will (hopefully) improve in the future which means that source code would have to be reformatted again.

I'm happy that you included reformatted source code in the PR though, as it makes it easy to see what style clang-format would produce.

Also, I wonder why BinPackArguments isn't enabled, that's kind of the policy we've been enforcing right?

indutny · 2015-10-31T08:05:04Z

@trevnorris we can make it put each argument on a newline, but I would like to say that IMO putting each argument on a new line is less readable.

However, I think we may go with the way we have things now (each argument on a new line) for now, just to postpone the decision on this to the later time. I think it is still very important to get proper formatting in our C++ code.

@piscisaureus this is actually a pretty interesting idea. However it does not help us to enforce a code style over the code base, unless we will use separate tools to verify that the code in commits is properly formatted.

Regarding, clang-format updates, we can fix the sha1 hash in core until some future time, thus making the code style quite stable.

I don't mind using BinPackArguments, looks like everyone wants to have it.

piscisaureus · 2015-10-31T08:13:41Z

@piscisaureus this is actually a pretty interesting idea. However it does not help us to enforce a code style over the code base, unless we will use separate tools to verify that the code in commits is properly formatted.

It's actually pretty easy! Just run git clang-format origin/master...HEAD. If the user did run it, clang-format should make no further changes, so we can simply whether the working tree is clean after running it.

indutny · 2015-10-31T08:26:17Z

Anyone want to give any arguments for or against @piscisaureus idea?

Fishrock123 · 2015-10-31T14:19:27Z

I'm no so sure about some of the styles enforced, can we configure that more, ala eslint?

jasnell · 2015-10-31T14:31:53Z

Can't say I'm a huge fan of this but I don't feel too strongly about it. I agree with @trevnorris and @Fishrock123 on not being happy with the styles enforced. I happen to like all my arguments separated out thank you very much ;-).

And if we are doing this, I'd prefer a much more incremental approach on rolling it out as @piscisaureus suggests. Just seems like a huge bit of churn for no obvious benefit... and definitely not convinced it should go into LTS but with this much code touched by the PR I don't think we'd have any choice (which isn't really a good thing)

19h · 2015-10-31T15:31:04Z

I really dislike this enforced formatting, because at more than just a few places in the diff of this PR the reformat just makes scanning the code extremely hard. The most obvious thing that bugs me is the merge of multi-line arguments to methods into a single line whereas the methods that are being called usually have arguments that are not obvious at all; they then require much more of a 'thorough look' in order to be properly understood.

e.g.

(1)

ares_query(env()->cares_channel(),
name,
ns_c_in,
ns_t_txt,
Callback,
GetQueryArg());

becoming

(2)

ares_query(env()->cares_channel(), name, ns_c_in, ns_t_txt, Callback,
GetQueryArg());

In my opinion, in (1) it is far easier to skip through the code vertically.. which might have something in common with my dislike of unnecessarily long lines.

$.02

trevnorris · 2015-10-31T15:48:37Z

@indutny FWIW I think having linting enabled would be a great thing. Seems the styles themselves just need to be hashed out.

I'm alright with the idea of incrementally adding rules that everyone can agree upon. Also with using git clang-format, but make lint will have issues if a rule is introduced that doesn't work with what we have today.

Also, I believe the eslint introduction on the JS side has caused a similar number of less than useful code changes than this.

indutny · 2015-10-31T17:44:49Z

I have just pushed BinPackArguments and BinPackParameters, it looks like the number of required changes has reduced significantly.

May I ask everyone to take another look?

kkoopa · 2015-11-01T19:20:19Z

You should not allow control-flow statements without blocks. When adding future lines, which inevitably will happen, one does not get additional noise of adding the now necessary block statements, nor the bugs that always come from forgetting them.

indutny · 2015-11-01T19:27:09Z

@kkoopa I don't think that mandatory blocks are in our code-style at the moment, nor I can see this in any of recent C++ changes.

kkoopa · 2015-11-01T19:30:57Z

Now would be a good opportunity to introduce that. Since there will be a bunch of changes due to style either way, the old excuse of "we don't do pure style changes" should not hold. Can anyone list good arguments against said change?

indutny · 2015-11-01T19:48:27Z

I don't have any arguments except that this is not a code style that I currently use. I guess the argument for it is that double goto bud in apple's code?

bnoordhuis · 2015-11-13T15:08:26Z

@Fishrock123 Try it for a while, I'll bet you end up liking it.

indutny · 2015-11-13T18:03:25Z

@Fishrock123 yep, it does. That's the idea, we should not really care about it anymore.

indutny · 2015-11-13T18:08:21Z

@bnoordhuis all fixed, please take a look.

bnoordhuis · 2015-11-13T21:21:03Z

Can I suggest calling this GIT_CLANG_FORMAT?

Fishrock123 · 2015-11-13T21:24:37Z

That's the idea, we should not really care about it anymore.

Umm, I'm not sure I understand, could you explain why, if you didn't already?

Fishrock123 · 2015-11-13T21:25:43Z

Also only linting new changes is confusing and seems even more inconsistent.

bnoordhuis · 2015-11-13T21:27:45Z

Not too many comments on the PR itself but a few questions about the approach:

Is auto-downloading a good thing? Maybe print an error instead and provide a make download target?
Is there a way around to checking in those downloader scripts? I'm guessing the answer is 'no' and that wget and shasum don't cut it but I have to ask.

bnoordhuis · 2015-11-13T21:31:06Z

@Fishrock123

Also only linting new changes is confusing and seems even more inconsistent.

The first version of this PR linted everything, resulting in a massive diff.

Umm, I'm not sure I understand, could you explain why, if you didn't already?

It's mechanizing what we do by hand now: sticking to the style guide.

Fishrock123 · 2015-11-13T21:32:43Z

Fair enough I suppose.

I still think having the linter autocorrect it for you is pretty aggressive and we shouldn't do that though.

jbergstroem · 2015-11-13T21:32:54Z

@bnoordhuis i've raised this [download stuff] as well. I'd rather see installing it documented since its pretty easy to install for mac (brew install clang-format), windows (part of llvm) and similar for linux/freebsd users. The git clang-format subset is only for developers, so making assumptions about them taking care of the stack should be fair.

bnoordhuis · 2015-11-13T21:39:02Z

It does make sense to stick to a single version of clang-format because it has changed dramatically since its introduction in 3.4, it's just that I'm not wild about auto-downloading it.

jbergstroem · 2015-11-13T21:44:29Z

@bnoordhuis so you're saying future changes are a problem? I just thought it was a cutoff at 3.5-3.6 (somewhere) where they massively changed it.

bnoordhuis · 2015-11-13T22:53:54Z

I don't have a llvm/clang source tree checked out on this machine so I can't check but IIRC there were quite a few changes and additions between 3.6 and 3.7 too. I haven't really tracked what's upcoming in 3.8.

trevnorris · 2015-11-16T23:28:07Z

We have an idea of how big the download is? Also, can I just use the included .clang-format style to see how it'll make the source look?

indutny · 2015-11-23T23:45:48Z

Ok, everyone, pushed update to the makefile. Now downloading clang-format is optional, and makefile will check the present version before using it. cc @jbergstroem

@trevnorris here are the forced changes of current .clang-format: https://gist.github.com/indutny/d35cdb1b73a1660ffe48 . You may get them yourself with clang-format -i src/*.cc src/*.h in the root of the project.

Add `format-commit` and `install-pre-commit-hook` make targets. First one will format all committed changes against current `.clang-format`. The latter one will execute `make format-commit` on `pre-commit` hook.

indutny · 2015-11-23T23:53:21Z

And updated the gist, I have changed AllowShortLoopsOnASingleLine to false.

indutny · 2015-12-01T06:30:50Z

Hitting one month on this PR. What is our decision on this?

Fishrock123 · 2015-12-01T15:58:38Z

-1 if it still autocorrects the linting

jasnell · 2015-12-01T17:05:35Z

@indutny , I definitely appreciate the work on this but I'm leaning -1 on this. I'm just not seeing the overall value at this point.

indutny · 2015-12-01T18:32:27Z

Ok... closing.

indutny added c++ Issues and PRs that require attention from people who are familiar with C++. build Issues and PRs related to build files or the CI. labels Oct 31, 2015

indutny force-pushed the feature/clang-format branch from 33a2c05 to e2c1224Compare October 31, 2015 17:44

bnoordhuis reviewed Nov 13, 2015
View reviewed changes

Comment threadMakefile Outdated
bnoordhuisNov 13, 2015
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I suggest calling this GIT_CLANG_FORMAT?

jasnell mentioned this pull request Nov 16, 2015
LTS WG Meeting - 2015-11-16 1pm Pacific Time nodejs/Release#56
Closed

indutny force-pushed the feature/clang-format branch 2 times, most recently from f88fe41 to 35ba5ffCompare November 23, 2015 23:43

make: format-commit for git-clang-format
02cbc0b
Add `format-commit` and `install-pre-commit-hook` make targets. First one will format all committed changes against current `.clang-format`. The latter one will execute `make format-commit` on `pre-commit` hook.

indutny force-pushed the feature/clang-format branch from 35ba5ff to 02cbc0bCompare November 23, 2015 23:52

indutny closed this Dec 1, 2015

MylesBorins mentioned this pull request Jan 11, 2016
LTS WG Meeting 2015-01-11 nodejs/Release#67
Closed

jasnell removed the lts-agenda label Jan 11, 2016

bnoordhuis mentioned this pull request Sep 21, 2017
crypto: make some easy parts 1.1.0-compatible #15348
Closed
2 tasks

Uh oh!

Uh oh!

Conversation

indutny commented Oct 31, 2015

Uh oh!

indutny commented Oct 31, 2015

Uh oh!

mscdex commented Oct 31, 2015

Uh oh!

jbergstroem commented Oct 31, 2015

Uh oh!

indutny commented Oct 31, 2015

Uh oh!

indutny commented Oct 31, 2015

Uh oh!

Qard commented Oct 31, 2015

Uh oh!

mikeal commented Oct 31, 2015

Uh oh!

mscdex commented Oct 31, 2015

Uh oh!

jbergstroem commented Oct 31, 2015

Uh oh!

indutny commented Oct 31, 2015

Uh oh!

mikeal commented Oct 31, 2015

Uh oh!

indutny commented Oct 31, 2015

Uh oh!

mikeal commented Oct 31, 2015

Uh oh!

trevnorris commented Oct 31, 2015

Uh oh!

piscisaureus commented Oct 31, 2015

Uh oh!

indutny commented Oct 31, 2015

Uh oh!

piscisaureus commented Oct 31, 2015

Uh oh!

indutny commented Oct 31, 2015

Uh oh!

Fishrock123 commented Oct 31, 2015

Uh oh!

jasnell commented Oct 31, 2015

Uh oh!

19h commented Oct 31, 2015• edited by gibfahn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trevnorris commented Oct 31, 2015

Uh oh!

indutny commented Oct 31, 2015

Uh oh!

kkoopa commented Nov 1, 2015

Uh oh!

indutny commented Nov 1, 2015

Uh oh!

kkoopa commented Nov 1, 2015

Uh oh!

indutny commented Nov 1, 2015

Uh oh!

bnoordhuis commented Nov 13, 2015

Uh oh!

indutny commented Nov 13, 2015

Uh oh!

indutny commented Nov 13, 2015

Uh oh!

bnoordhuisNov 13, 2015

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Nov 13, 2015

Uh oh!

Fishrock123 commented Nov 13, 2015

Uh oh!

bnoordhuis commented Nov 13, 2015

Uh oh!

bnoordhuis commented Nov 13, 2015

Uh oh!

Fishrock123 commented Nov 13, 2015

Uh oh!

jbergstroem commented Nov 13, 2015

19h commented Oct 31, 2015•
edited by gibfahn
Loading