Fixed cpplint issues #63
Conversation
| v8::Handle<T> val | ||
| )) { | ||
| //TODO: remove <0.11.9 support when 0.12 is released | ||
| v8::Handle<T> val) |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
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. |
|
I patched cpplint.py, sadly it suppressed a bit more than wished for. Updated PR. |
|
You can suppress warnings from within the source code with a The check list can be configured with the --filter argument to cpplint, e.g. |
| @@ -293,7 +293,7 @@ static NAN_INLINE(uint32_t NanUInt32OptionValue( | |||
| , v8::Persistent<v8::Object>* object \ | |||
| , type data) | |||
| # define NAN_WEAK_CALLBACK_OBJECT (*object) | |||
| # define NAN_WEAK_CALLBACK_DATA(type) ((type) data) | |||
| # define NAN_WEAK_CALLBACK_DATA(type) (reinterpret_cast<type>(data)) | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
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. |
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 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. |
|
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. |
|
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. |
|
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. |
|
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. |
|
@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.
I don't disagree but think of it as an improvement on the current state of affairs. :-) |
Add a Makefile that runs the linter over source and header files. Refs nodejs#63.
|
Finally got around to this. Now everything passes the cpplint and compiles. |
|
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. |
|
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. |
|
Is there anything blocking this? If you were holding out for a LGTM then LGTM, squash and land at will. |
|
Included in #64 |
cpplint suggested these changes