doc,meta: add references to outside C++ guides #23317

refack · 2018-10-07T17:16:56Z

Suggested words about outside coding guidelines, and set their priorities.
Fixes: #22862

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-10-07T17:16:57Z

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1158/pipeline

joyeecheung · 2018-10-07T20:40:29Z

CPP_STYLE_GUIDE.md

+    * [Memory allocation](#memory-allocation)
+    * [Use `nullptr` instead of `NULL` or `0`](#use-nullptr-instead-of-null-or-0)
+    * [Ownership and Smart Pointers](#ownership-and-smart-pointers)
+  * [Others](#others)


Somehow I feel like this should be Semantics and Memory Management can be its own section?
(Also ideally we can have a section dedicated to C++/JS interactions and V8/libuv usage)

joyeecheung · 2018-10-07T20:43:08Z

CPP_STYLE_GUIDE.md

-### Memory allocation
+## Semantics
+
+The Node.js C & C++ code strives to be consistent in it's use of language


It's a bit odd to put this section here...can we move it to the beginning of the document? We do inherit some formatting styles from Google's C++ style anyway.

Ack x 2.
I'll try to reformat this.

joyeecheung · 2018-10-07T20:44:30Z

CPP_STYLE_GUIDE.md

+
+In general code should follow the CPPCG, unless overridden by the GCSG or this
+document. ATM these guidelines are checked manually by reviewers, but the goal
+is to automate this check via tools such as `clang-tidy`.


I wouldn't mention clang-tidy as a goal here since the last time I attempted to use it on our code base it took really long to complete linting...certainly much longer than cpplint and cpplint is already kind of slow.

Ack.
Recent versions seems to work much much better. MSVS has it built in, and it runs in reasonable time (for just /src/ and with a limited ruleset). It does spit out a huge amount of recommendations.

joyeecheung · 2018-10-07T20:45:26Z

CPP_STYLE_GUIDE.md

+3. The ISO [C++ Core Guidelines][](CPPCG)
+
+In general code should follow the CPPCG, unless overridden by the GCSG or this
+document. ATM these guidelines are checked manually by reviewers, but the goal


s/ATM/At the moment/

automatic teller machine*

addaleax · 2018-10-07T23:11:32Z

CPP_STYLE_GUIDE.md

+Unfortunately, the C++ linter (based on [Google’s `cpplint`][]), which can be
+run explicitly via `make lint-cpp`, does not currently catch a lot of rules that
+are specific to the Node.js C++ code base. This document explains the most
+common of these rules:


If you want, you can remove this paragraph or merge it into the previous one, since it shares a lot of substance with it.

Eventually, I decided to keep it as is. I think it's better to keep semantic from formatting separate, even in this case.

addaleax · 2018-10-07T23:12:35Z

CPP_STYLE_GUIDE.md

+2. The [Google C++ Style Guide][](GCSG)
+3. The ISO [C++ Core Guidelines][](CPPCG)
+
+In general code should follow the CPPCG, unless overridden by the GCSG or this


I’d prefer to spell out the abbreviations.

Also, maybe make it a bit clearer that our style guide is closest to the Google one?

vsemozhetbyt · 2018-10-08T19:55:18Z

CPP_STYLE_GUIDE.md

+features and idioms, as well as have some specific guidelines for the use of
+runtime features.
+
+Coding guidelines are based of following guides (highest priority first):


based of -> based on?

vsemozhetbyt · 2018-10-08T19:56:41Z

CPP_STYLE_GUIDE.md

+
+Coding guidelines are based of following guides (highest priority first):
+1. This document
+2. The [Google C++ Style Guide][](GCSG)


A space between the link and (GCSG)?

vsemozhetbyt · 2018-10-08T19:56:53Z

CPP_STYLE_GUIDE.md

+Coding guidelines are based of following guides (highest priority first):
+1. This document
+2. The [Google C++ Style Guide][](GCSG)
+3. The ISO [C++ Core Guidelines][](CPPCG)


with explicit priorities PR-URL: nodejs#23317 Reviewed-By: Anna Henningsen <[email protected]>

refack · 2018-10-13T22:08:46Z

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1237/

addaleax · 2018-10-13T22:21:33Z

@refack In case you missed it: #22255

refack · 2018-10-13T22:24:08Z

It's a week old.
Ohh, it's just 6 days, my bad.

with explicit priorities PR-URL: #23317 Reviewed-By: Anna Henningsen <[email protected]>

refack added c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations. discuss Issues opened for discussions and feedbacks. meta Issues and PRs related to the general management of the project. labels Oct 7, 2018

refack self-assigned this Oct 7, 2018

refack requested review from addaleax and joyeecheung October 7, 2018 17:16

refack mentioned this pull request Oct 7, 2018

src: reduce scope of variables throughout codebase #23297

Merged

2 tasks

joyeecheung reviewed Oct 7, 2018

View reviewed changes

refack force-pushed the cpp-guideline-references branch from 5e72e11 to 546a42f Compare October 7, 2018 23:08

addaleax approved these changes Oct 7, 2018

View reviewed changes

vsemozhetbyt reviewed Oct 8, 2018

View reviewed changes

doc,meta: add references to outside C++ guides

0f8eaa4

with explicit priorities PR-URL: nodejs#23317 Reviewed-By: Anna Henningsen <[email protected]>

refack force-pushed the cpp-guideline-references branch from 4f9a027 to 0f8eaa4 Compare October 13, 2018 22:08

refack removed the discuss Issues opened for discussions and feedbacks. label Oct 13, 2018

refack merged commit 0f8eaa4 into nodejs:master Oct 13, 2018

refack deleted the cpp-guideline-references branch October 13, 2018 22:15

addaleax pushed a commit that referenced this pull request Oct 14, 2018

doc,meta: add references to outside C++ guides

a0c8f85

with explicit priorities PR-URL: #23317 Reviewed-By: Anna Henningsen <[email protected]>

jasnell pushed a commit that referenced this pull request Oct 17, 2018

doc,meta: add references to outside C++ guides

cabf144

with explicit priorities PR-URL: #23317 Reviewed-By: Anna Henningsen <[email protected]>

refack removed their assignment Oct 20, 2018

MylesBorins pushed a commit that referenced this pull request Oct 30, 2018

doc,meta: add references to outside C++ guides

2f51e49

with explicit priorities PR-URL: #23317 Reviewed-By: Anna Henningsen <[email protected]>

refack changed the title ~~[WIP] doc,meta: add references to outside C++ guides~~ doc,meta: add references to outside C++ guides Nov 13, 2018

refack mentioned this pull request Nov 13, 2018

c++: Adopt the CppCoreGuidelines #22862

Closed

codebytere mentioned this pull request Nov 27, 2018

v10.13.1 proposal #24675

Closed

rvagg pushed a commit that referenced this pull request Nov 28, 2018

doc,meta: add references to outside C++ guides

8d3af46

with explicit priorities PR-URL: #23317 Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins pushed a commit that referenced this pull request Nov 29, 2018

doc,meta: add references to outside C++ guides

04f27aa

with explicit priorities PR-URL: #23317 Reviewed-By: Anna Henningsen <[email protected]>

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc,meta: add references to outside C++ guides #23317

doc,meta: add references to outside C++ guides #23317

refack commented Oct 7, 2018 •

edited

Loading

nodejs-github-bot commented Oct 7, 2018

joyeecheung Oct 7, 2018

joyeecheung Oct 7, 2018

addaleax Oct 7, 2018

refack Oct 7, 2018

joyeecheung Oct 7, 2018

refack Oct 7, 2018

joyeecheung Oct 7, 2018

refack Oct 7, 2018

devsnek Oct 7, 2018

addaleax Oct 7, 2018

refack Oct 13, 2018

addaleax Oct 7, 2018

vsemozhetbyt Oct 8, 2018

vsemozhetbyt Oct 8, 2018

vsemozhetbyt Oct 8, 2018

refack commented Oct 13, 2018

addaleax commented Oct 13, 2018

refack commented Oct 13, 2018

doc,meta: add references to outside C++ guides #23317

doc,meta: add references to outside C++ guides #23317

Conversation

refack commented Oct 7, 2018 • edited Loading

Checklist

nodejs-github-bot commented Oct 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Oct 13, 2018

addaleax commented Oct 13, 2018

refack commented Oct 13, 2018

refack commented Oct 7, 2018 •

edited

Loading