doc: cleanup and add references to CPP_STYLE_GUIDE.md #23650

refack · 2018-10-14T01:07:09Z

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-14T01:07:10Z

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

richardlau · 2018-10-14T01:21:16Z

CPP_STYLE_GUIDE.md

 not inside of nested calls.

-Using C++ `throw` is not allowed.
+Node.js is built without C++ exception semantics, so code using `thorw` or, even


thorw->throw

richardlau · 2018-10-14T01:21:59Z

CPP_STYLE_GUIDE.md

 ```cpp
 std::unique_ptr<Foo> FooFactory();
 void FooConsumer(std::unique_ptr<Foo> ptr);
 ```
+Since `std::unique_ptr` has only move semantics, passing one by value trasfers


trasfers->transfers

addaleax · 2018-10-14T03:03:24Z

CPP_STYLE_GUIDE.md

@@ -316,13 +311,19 @@ exports.foo = function(str) {

 #### Avoid throwing JavaScript errors in nested C++ methods

-When you have to throw the errors from C++, try to do it at the top level and
+When you need to return an error from C++, try to do it at the top level and


This referred specifically to throwing exceptions, though.

(s/error/exception/ would be a good change here, that’s on me.)

Can you clear this up for me? we can't c++ throw, we need to isolate()->ThrowException() right?

addaleax · 2018-10-14T03:04:00Z

CPP_STYLE_GUIDE.md

- Use `static_cast` for casting whenever it works
- `reinterpret_cast` is okay if `static_cast` is not appropriate
+- Use `static_cast<T>` if casting is required, and its valid 
+- Use `reinterpret_cast` only when its essential


typo: it is (twice)

(and maybe s/essential/necessary/)

addaleax · 2018-10-14T03:05:04Z

CPP_STYLE_GUIDE.md


-Never use `std::auto_ptr`. Instead, use `std::unique_ptr`.
+Avoid `std::auto_ptr`. It was deprecated in C++11 and removed from C++17.


This seemed sensible in previous form? There’s no reason to use it in our code, and that it’s deprecated/removed is not really of concern if you already know not to use it

I didn't like the phrasing "Never use", also I do believe an explanation why is nice, since it's very short.

Maybe:

Don't use `std::auto_ptr`.<br/> It was deprecated in C++11 if favor of `std::unique_ptr`, and later removed from C++17. `std::unique_ptr` has move semantics instead the of unusual copy semantics of `std::auto_ptr`.

Actually we should be able to lint for that. I'll see if I can figure out how to.

addaleax · 2018-10-14T03:05:49Z

CPP_STYLE_GUIDE.md

 ```cpp
 std::unique_ptr<Foo> FooFactory();
 void FooConsumer(std::unique_ptr<Foo> ptr);
 ```
+Since `std::unique_ptr` has only move semantics, passing one by value trasfers


nit: blank lines around code block

addaleax · 2018-10-14T03:07:27Z

CPP_STYLE_GUIDE.md

@@ -192,40 +192,35 @@ class FancyContainer {

 ### Use `nullptr` instead of `NULL` or `0`

-What it says in the title.
+Further reading at [ES.47].


I’d really like it if we could make these links more expressive instead of just having the item numbers in the text, e.g. Further reading in the [C++ Core Guidelines][ES.47]?

vsemozhetbyt · 2018-10-14T10:04:10Z

CPP_STYLE_GUIDE.md

-
-Prefer to use `std::unique_ptr` to make ownership
-transfer explicit. For example:
+[R.20]: Use `std::unique_ptr` or `std::shared_ptr` to represent ownership


Currently, this and next lines are rendered as one line.

CPP_STYLE_GUIDE.md

addaleax · 2018-10-14T19:41:17Z

CPP_STYLE_GUIDE.md

-Prefer to use `std::unique_ptr` to make ownership
-transfer explicit. For example:
+[R.20]: Use `std::unique_ptr` or `std::shared_ptr` to represent ownership<br/>
+[R.21]: Prefer `unique_ptr` over `shared_ptr` unless you need to share ownership


If you want a list of things, you can juse start lines with * instead of manually inserting line breaks

addaleax · 2018-10-14T19:41:32Z

CPP_STYLE_GUIDE.md

+Since `std::unique_ptr` has only move semantics, passing one by value transfers
+ownership to the callee and invalidates the instance the caller hold.
+
+Don't use `std::auto_ptr`.<br/>


No need for a line break, this makes sense as one paragraph.

addaleax · 2018-10-14T19:41:50Z

CPP_STYLE_GUIDE.md

+
+Further reading:<br/>
+[ES.48]: Avoid casts<br/>
+[ES.49]: If you must use a cast, use a named cast


refack · 2018-10-14T22:46:45Z

New format:

Explicit <br/> format:

🤔

addaleax

Thanks!

joyeecheung · 2018-10-14T22:53:16Z

CPP_STYLE_GUIDE.md

@@ -316,13 +316,19 @@ exports.foo = function(str) {

 #### Avoid throwing JavaScript errors in nested C++ methods

-When you have to throw the errors from C++, try to do it at the top level and
+When you need to throw an exception from C++, try to do it at the top level and


This somehow reads a bit ambiguous to me now that it's exception, can you change it to JavaScript exception?

joyeecheung · 2018-10-14T23:00:45Z

CPP_STYLE_GUIDE.md

+Since `std::unique_ptr` has only move semantics, passing one by value transfers
+ownership to the callee and invalidates the instance the caller hold.
+
+Don't use `std::auto_ptr`. It was deprecated in C++11 if favor of


Do we need to explain this here? Can we link to some docs somewhere?

Ack (ish). PTAL

refack · 2018-10-15T00:45:35Z

CPP_STYLE_GUIDE.md

+When you need to throw a JavaScript exception from C++ (i.e.
+`isolate()->ThrowException()`) prefer to do it as close to the return to JS as
+possible, and not inside of nested C++ calls. Since this changes the `isolate`'s
+state doing closest to where it is consumed, reduces the chance of side effects.


@joyeecheung, @addaleax PTAL
I'm trying to grok this, so I wrote it down.

s/isolate/Isolate/, and the comma should be removed

I don’t think it matters that it’s an Isolate thing, though. That’s implementation details, and you can just say “JS execution state” or something like that.

addaleax · 2018-10-15T00:47:35Z

CPP_STYLE_GUIDE.md


-Using C++ `throw` is not allowed.
+Node.js is built without C++ exception semantics, so code using `throw` or, even


s/exception semantics/exceptions/?

I went with exception handling and added a reference (GCC state 7% runtime cost which I think is interesting).

addaleax · 2018-10-15T00:48:08Z

CPP_STYLE_GUIDE.md

+When you need to throw a JavaScript exception from C++ (i.e.
+`isolate()->ThrowException()`) prefer to do it as close to the return to JS as
+possible, and not inside of nested C++ calls. Since this changes the `isolate`'s
+state doing closest to where it is consumed, reduces the chance of side effects.


s/isolate/Isolate/, and the comma should be removed

I don’t think it matters that it’s an Isolate thing, though. That’s implementation details, and you can just say “JS execution state” or something like that.

refack · 2018-10-16T15:32:35Z

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

PR-URL: nodejs#23650 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

PR-URL: #23650 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 14, 2018

refack requested review from addaleax and joyeecheung October 14, 2018 01:07

refack self-assigned this Oct 14, 2018

refack added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 14, 2018

richardlau reviewed Oct 14, 2018

View reviewed changes

addaleax reviewed Oct 14, 2018

View reviewed changes

vsemozhetbyt reviewed Oct 14, 2018

View reviewed changes

CPP_STYLE_GUIDE.md Show resolved Hide resolved

addaleax approved these changes Oct 14, 2018

View reviewed changes

joyeecheung approved these changes Oct 14, 2018

View reviewed changes

refack commented Oct 15, 2018

View reviewed changes

addaleax approved these changes Oct 15, 2018

View reviewed changes

refack force-pushed the more-references-in-cpp-guide branch from 41872c5 to 5764fdb Compare October 16, 2018 15:32

refack added the meta Issues and PRs related to the general management of the project. label Oct 16, 2018

doc: cleanup and references in C++ guide

cf3f8dd

PR-URL: nodejs#23650 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

refack force-pushed the more-references-in-cpp-guide branch from 5764fdb to cf3f8dd Compare October 16, 2018 21:50

refack merged commit cf3f8dd into nodejs:master Oct 16, 2018

refack deleted the more-references-in-cpp-guide branch October 16, 2018 22:05

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

doc: cleanup and references in C++ guide

9c5ec79

PR-URL: #23650 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

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

doc: cleanup and references in C++ guide

c98496d

PR-URL: #23650 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

refack removed their assignment Oct 20, 2018

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

doc: cleanup and references in C++ guide

3e4032a

PR-URL: #23650 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

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: cleanup and references in C++ guide

3ba7557

PR-URL: #23650 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

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

doc: cleanup and references in C++ guide

2a19518

PR-URL: #23650 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged


		Never use `std::auto_ptr`. Instead, use `std::unique_ptr`.
		Avoid `std::auto_ptr`. It was deprecated in C++11 and removed from C++17.


		Using C++ `throw` is not allowed.
		Node.js is built without C++ exception semantics, so code using `throw` or, even

doc: cleanup and add references to CPP_STYLE_GUIDE.md #23650

doc: cleanup and add references to CPP_STYLE_GUIDE.md #23650

Conversation

refack commented Oct 14, 2018

Checklist

nodejs-github-bot commented Oct 14, 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

refack Oct 14, 2018 • edited Loading

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

vsemozhetbyt Oct 14, 2018 • edited Loading

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 14, 2018

addaleax left a 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 16, 2018 • edited Loading

refack Oct 14, 2018 •

edited

Loading

vsemozhetbyt Oct 14, 2018 •

edited

Loading

refack commented Oct 16, 2018 •

edited

Loading