doc: add C++ style comments to the style guide #17617

mmarchini · 2017-12-11T19:44:35Z

Adds instructions on how to format C++ comments to the C++ style guide, as cpplint.py doesn't complain about C-style comments on the code and C++ style comments are the encouraged format.

There's a lot of C-style comments on the current code, and because of that new contributors may think C-style comments are the encouraged format for multi-line comments.

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

addaleax · 2017-12-11T20:28:33Z

There's a lot of C-style comments on the current code, and because of that new contributors may think C-style comments are the encouraged format for multi-line comments.

Are they discouraged by us (i.e. did somebody mention this as part of a review) or by somebody else?

I do sometimes like to use C style comments, and I've found that I tend to use C++ style comments for referring to the immediately following next line or two, and C style comments when referring to larger pieces of code. (And, although this is purely subjective observation, I think I'm not alone in that.)

bnoordhuis · 2017-12-11T20:56:36Z

Are they discouraged by us (i.e. did somebody mention this as part of a review) or by somebody else?

I usually request that people change them. 95% of our comments are C++-style so I'd like to see the other 5% go away over time. Many are from when node was still C-with-classes.

mmarchini · 2017-12-11T22:15:36Z

Are they discouraged by us (i.e. did somebody mention this as part of a review) or by somebody else?

@bnoordhuis requested me to change from C to C++ style in some PRs. I didn't mind changing it and I think it's good to have a defined style for comments, but I thought C++ style was already an established standard in Node.

Maybe we should decide which style to follow before including it into CPP_STYLE_GUIDE.md, but it would be good to have a comment style documented to help new contributors to know how to format their comments.

refack · 2017-12-11T22:36:26Z

CPP_STYLE_GUIDE.md

@@ -33,6 +34,21 @@ these rules:

 `char* buffer;` instead of `char *buffer;`

+## C++ style comments
+
+Always use C++ style comments (`//`) for single line and multi-line comments.


I would remove the Always, and add a comment that there might still be "old" comment in the code:

Use C++ style comments (`//`) for both single line and multi-line comments. Examples: ... The codebase may contain old C style comments (`/* */`) from before this was the prefered style.

mmarchini · 2017-12-12T19:13:42Z

PR updated. Thanks for the suggestion @refack

BridgeAR

LGTM but I would really like to add the upper case part and that the comments here start upper case as well. I always have to hold myself back not to complain about it when I do code reviews.

BridgeAR · 2017-12-12T20:08:47Z

CPP_STYLE_GUIDE.md

+Examples:
+
+```c++
+// a single line comment


When we are on it - it would be nice to add that the start of a comment should also be upper cased 😄

And a dot at the end!

mmarchini · 2017-12-13T13:25:33Z

@BridgeAR @bnoordhuis something like that?

## C++ style comments

Use C++ style comments (`//`) for both single line and multi-line comments.
Comments should also start with uppercase and finish with a dot.

Examples:

```c++
// A single line comment.


// Multi-line comments
// should also follow
// C++ style comments.
```

The codebase may contain old C style comments (`/* */`) from before this was the
preferred style.

Maybe we could also encourage people to update the style of comments on old code when working close to it?

The codebase may contain old C style comments (`/* */`) from before this was the
preferred style. Feel free to update old comments to the preferred style when working
on code close to it or when changing/improving those comments.

bnoordhuis · 2017-12-13T13:57:59Z

Maybe change 'close to' to the more specific 'in the immediate vicinity of', otherwise sounds good.

A rule of thumb I use is that when a purely stylistic change results in a new hunk in the diff, it ain't worth doing.

cjihrig · 2017-12-13T14:04:40Z

CPP_STYLE_GUIDE.md

+
+
+// Multi-line comments
+// should also follow


follow -> use

mmarchini · 2017-12-13T21:08:18Z

Pull request updated with latest suggestions

apapirovski

Just some nits.

apapirovski · 2017-12-14T15:14:51Z

CPP_STYLE_GUIDE.md

+
+
+// Multi-line comments
+// should also use  C++


can we remove the double space before C++.

apapirovski · 2017-12-14T15:14:53Z

CPP_STYLE_GUIDE.md

+
+The codebase may contain old C style comments (`/* */`) from before this was the
+preferred style. Feel free to update old comments to the preferred style when
+working on code in the immediate vicinity of it or when changing/improving those


s/immediate vicinity of it/immediate vicinity

apapirovski · 2017-12-14T15:15:52Z

CPP_STYLE_GUIDE.md

+```c++
+// A single line comment.
+
+


The double line break feels unnecessary here.

apapirovski · 2017-12-14T15:18:00Z

CPP_STYLE_GUIDE.md

@@ -33,6 +34,27 @@ these rules:

 `char* buffer;` instead of `char *buffer;`

+## C++ style comments
+
+Use C++ style comments (`//`) for both single line and multi-line comments.


Here and elsewhere, s/single line/single-line.

Adds instructions on how to format C++ comments to the C++ style guide, as `cpplint.py` doesn't complain about C-style comments on the code.

mmarchini · 2017-12-14T19:21:22Z

@apapirovski done :)

apapirovski · 2017-12-14T19:22:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request-lite/20/

Adds instructions on how to format C++ comments to the C++ style guide, as `cpplint.py` doesn't complain about C-style comments on the code. PR-URL: nodejs#17617 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

BridgeAR · 2017-12-15T21:40:27Z

Landed in a17f426

Adds instructions on how to format C++ comments to the C++ style guide, as `cpplint.py` doesn't complain about C-style comments on the code. PR-URL: #17617 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Dec 11, 2017

addaleax added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 11, 2017

refack reviewed Dec 11, 2017

View reviewed changes

mmarchini force-pushed the docs/cpp_style_comment branch from 50b5992 to 3a1d18f Compare December 12, 2017 19:12

BridgeAR approved these changes Dec 12, 2017

View reviewed changes

jasnell approved these changes Dec 12, 2017

View reviewed changes

mmarchini force-pushed the docs/cpp_style_comment branch from 3a1d18f to 69dc7ac Compare December 13, 2017 13:12

cjihrig approved these changes Dec 13, 2017

View reviewed changes

CPP_STYLE_GUIDE.md Outdated

// Multi-line comments

// should also follow

Copy link

Contributor

cjihrig Dec 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow -> use

mmarchini force-pushed the docs/cpp_style_comment branch from 69dc7ac to 6a0c309 Compare December 13, 2017 21:06

bnoordhuis approved these changes Dec 14, 2017

View reviewed changes

apapirovski approved these changes Dec 14, 2017

View reviewed changes

doc: add C++ style comments to the style guide

8e7a098

Adds instructions on how to format C++ comments to the C++ style guide, as `cpplint.py` doesn't complain about C-style comments on the code.

mmarchini force-pushed the docs/cpp_style_comment branch from 6a0c309 to 8e7a098 Compare December 14, 2017 19:20

apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 14, 2017

danbev approved these changes Dec 15, 2017

View reviewed changes

BridgeAR closed this Dec 15, 2017

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added dont-land-on-v6.x labels Jan 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: add C++ style comments to the style guide #17617

doc: add C++ style comments to the style guide #17617

mmarchini commented Dec 11, 2017

addaleax commented Dec 11, 2017

bnoordhuis commented Dec 11, 2017

mmarchini commented Dec 11, 2017

refack Dec 11, 2017

mmarchini commented Dec 12, 2017

BridgeAR left a comment

BridgeAR Dec 12, 2017

bnoordhuis Dec 13, 2017

mmarchini commented Dec 13, 2017 •

edited

Loading

bnoordhuis commented Dec 13, 2017

cjihrig Dec 13, 2017

mmarchini commented Dec 13, 2017

apapirovski left a comment

apapirovski Dec 14, 2017

apapirovski Dec 14, 2017

apapirovski Dec 14, 2017

apapirovski Dec 14, 2017

mmarchini commented Dec 14, 2017

apapirovski commented Dec 14, 2017

BridgeAR commented Dec 15, 2017

doc: add C++ style comments to the style guide #17617

doc: add C++ style comments to the style guide #17617

Conversation

mmarchini commented Dec 11, 2017

Checklist

Affected core subsystem(s)

addaleax commented Dec 11, 2017

bnoordhuis commented Dec 11, 2017

mmarchini commented Dec 11, 2017

Choose a reason for hiding this comment

mmarchini commented Dec 12, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmarchini commented Dec 13, 2017 • edited Loading

bnoordhuis commented Dec 13, 2017

Choose a reason for hiding this comment

mmarchini commented Dec 13, 2017

apapirovski 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

mmarchini commented Dec 14, 2017

apapirovski commented Dec 14, 2017

BridgeAR commented Dec 15, 2017

mmarchini commented Dec 13, 2017 •

edited

Loading