linter: add cpplint

[gdams]

This PR adds cpplint to linter all the c/c++ files. It can be run from
npm test or directly from npm run lint. There are still a few linter errors which I have included below:

 ✗ src/module.cc
   3: Include the directory when naming .h files
   16: Using C-style cast.  Use static_cast<int>(...) instead
   266: Using C-style cast.  Use static_cast<int>(...) instead
 ✗ src/node_report.cc
   2: Include the directory when naming .h files
   15: Include the directory when naming .h files
   16: Include the directory when naming .h files
   365: Using C-style cast.  Use reinterpret_cast<char ***>(...) instead
   589: Using C-style cast.  Use reinterpret_cast<LPBYTE *>(...) instead
   1038: Storage class (static, extern, typedef, etc) should be first.
   1096: Using C-style cast.  Use reinterpret_cast<FILE*>(...) instead
   1120: Using C-style cast.  Use reinterpret_cast<char*>(...) instead
   1129: Using C-style cast.  Use reinterpret_cast<char*>(...) instead
   1138: Using C-style cast.  Use reinterpret_cast<ld_info*>(...) instead
   1164: Using C-style cast.  Use reinterpret_cast<HMODULE*>(...) instead
   364: Add #include <string> for string
 ✗ src/node_report.h
   5: Include the directory when naming .h files
   7: Found C system header after other header. Should be: node_report.h, c system, c++ system, other.
   8: Found C system header after other header. Should be: node_report.h, c system, c++ system, other.
   10: Found C system header after other header. Should be: node_report.h, c system, c++ system, other.

[gdams]

so I have added the cpplint.py to run the python suite that node.js use: https://github.com/nodejs/node/blob/master/tools/cpplint.py

[indutny]

I wonder if there is any reason not to use clang-format in this project? Linting is good, but fixing issues with one commands is better in my opinion

[hhellyer]

Having had a local discussion about this I think the concern was that clang-format wouldn't be available when running citgm builds. (I'd be happy with something that a developer just ran before creating a PR personally but I can see why other people want to integrate it into builds.)

I think the main concern with this pull request at the moment is cpplint.py is 6153 lines long as it's copied from Node.js it's likely to get out of sync.

[gibfahn]

I think ideally we want an npm module we can just have as a devDependency, one that can automatically fix stuff would be great. Not sure if there are any good ones though.

[richardlau]

My main objection to clang-format is that it isn't part of the standard Node.js addon development environment on platforms such as Windows nor is it (I believe) an installable npm module.

@gdams Where is this version of cpplint.py from? It doesn't match the one currently in https://github.com/nodejs/node/blob/master/tools/cpplint.py (e.g. missing nodejs/node@fadf66a).

[richardlau]

Can you rebase on top of master now that #52 has landed?

[richardlau]

Any thoughts on what the copyright attribution should be? @nodejs/post-mortem

[sam-github]

@jasnell you may be bringing all Node.js src copyright statements into alignment with legal requirements, opinions on this?

[mhdawson]

I'll et @jasnell confirm but I think it should be:

// Copyright node-report contributors. All rights reserved.
// SPDX-License-Identifier: MIT

in line with https://github.com/nodejs/node/pull/10599/files as these are "new" files that were never under the joyent copyright.

[gibfahn]

I think there was also some discussion about removing the All rights reserved from that.

[richardlau]

Looks like cpplint.py should be in a tools/ directory. Can you move it?

[sam-github]

I've had trouble getting clang-format installed when we used it once before for formatting checking. IIRC, it also had continuously changing formats, so you needed the exact correct version installed. The pain outweighed the gain.

This PR LGTM, except that the copyright header comments don't refer to anyone who can hold copyright... does the linter require them?

[sam-github]

Sorry, just realized, this should be:

   "test": "tap --timeout 10 test/test*.js",
   "pretest": "python cpplint.py **/*.cc **/*.h"

And the tap timeout change is unrelated to this PR, shouldn't it be a different PR?

[gibfahn]

Why have that as pretest? It means that you can't just do npm run lint to lint your code.