[kkoopa]

cpplint suggested these changes

[kkoopa]

I don't know how easy it is to automatically ignore stuff. It's just one python file http://google-styleguide.googlecode.com/svn/trunk/cpplint/cpplint.py which I have not looked at, but it should be possible to patch it. cpplint is a google thing, so it contains a bunch of their own style policies as well as more general things.

Read the file, seems possible to do suppression of sorts. I'll experiment a bit.

[kkoopa]

I patched cpplint.py, sadly it suppressed a bit more than wished for. Updated PR.

[bnoordhuis]

You can suppress warnings from within the source code with a // NOLINT or // NOLINT(whitespace/parens) comment. The second form is preferred, a bare NOLINT suppresses all warnings on that line.

The check list can be configured with the --filter argument to cpplint, e.g. --filter=-whitespace/parens to disable the whitespace/parens warning or --filter=-,+legal/copyright to disable everything except the legal/copyright warning.

[rvagg]

I think I might have been too assertive here. I want you guys to feel some ownership of the project and I'm not a fan of the BDFL leadership model. As with my other "open" projects, I'm happy to be outvoted if there is a prevailing opinion that I don't share.

On this particular topic: obviously there is an existing coding style and cpplint is clashing with it. I'd personally not change the style because someone's opinionated linter suggests it. However I do recognise I tend to adopt a fairly non-idiomatic C++ style.

So fwiw, questions of style are still fair game if you feel strongly enough about it. You just have to convince enough active contributors of your opinions before making a change and not just changing on a whim or for change's sake.

[bnoordhuis]

On this particular topic: obviously there is an existing coding style and cpplint is clashing with it. I'd personally not change the style because someone's opinionated linter suggests it. However I do recognise I tend to adopt a fairly non-idiomatic C++ style.

I don't really have a strong opinion on code style but using a linter tends to squelch formatting disputes in the bud. We can add a Makefile that runs cpplint with disagreeable warnings disabled (like whitespace/parens) and tell people to run make lint before they submit a PR.

The non-whitespace changes in this PR look okay to me. Replacing C casts with C++ casts, marking single-argument constructors as explicit, etc. - they're all changes that help avoid bugs.

[brett19]

I agree with bnoordhuis on this, I personally don't like some of the style (space between function name and function parameter list in a function declaration for instance), but I believe that using automatic code formatting can be really helpful to ensure the code stays consistent across a number of different contributors and helps identify or fix small potential bugs.

[kkoopa]

As the others have said, I don't care greatly about style either -- as long as it passes some validator and is consistent. Cpplint is the most practical one I know of at the moment.

[bnoordhuis]

Any progress on this? I'd suggest undoing the whitespace/parens changes and adding a lint Makefile target that suppresses that warning. I've opened #65 with a first pass that you can incorporate.

[kkoopa]

The only problem with that solution is that it suppresses this checking everywhere, else I would have made use of it sooner, but I guess it is better than the alternatives.

[bnoordhuis]

@kkoopa See @rvagg's LGTM on #65. I think if you incorporate bnoordhuis/nan@2ff7517 into this PR and undo the NOLINT(whitespace/parens) changes, then it's good to go.

The only problem with that solution is that it suppresses this checking everywhere, else I would have made use of it sooner, but I guess it is better than the alternatives.

I don't disagree but think of it as an improvement on the current state of affairs. :-)

[kkoopa]

Finally got around to this. Now everything passes the cpplint and compiles.

[bnoordhuis]

Apropos the license boilerplate, I wouldn't bother with individual attribution, just make it something like "Copyright 2014, the NAN contributors." Individual attribution makes for tedious updates when a new maintainer comes on board.

Another thing about the boilerplate, it's customary (though not ubiquitous) to include the license rather than linking to it. Not a big issue though.

[kkoopa]

Agreed. I removed the individual contributor list from all but the main file. I think a link to the license will be fine. Don't see the point of adding more clutter.

[bnoordhuis]

Is there anything blocking this? If you were holding out for a LGTM then LGTM, squash and land at will.

[kkoopa]

Included in #64