-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
doc: introduce categories to Cpp style guide #17095
Conversation
CPP_STYLE_GUIDE.md
Outdated
|
||
Unfortunately, the C++ linter (based on | ||
[Google’s `cpplint`](https://github.com/google/styleguide)), 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: | ||
|
||
# Formatting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be ##?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixed
CPP_STYLE_GUIDE.md
Outdated
* [Type casting](#type-casting) | ||
* [Memory allocation](#memory-allocation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put "Others" as a top level item and put other rules under it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
CPP_STYLE_GUIDE.md
Outdated
- `dynamic_cast` does not work because RTTI is not enabled | ||
- Use `static_cast` for casting whenever it works | ||
- `reinterpret_cast` is okay if `static_cast` is not appropriate | ||
# Memory Management |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also should be ##
CPP_STYLE_GUIDE.md
Outdated
|
||
Never use `std::auto_ptr`. Instead, use `std::unique_ptr`. | ||
|
||
# Others |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
ae8a2e8
to
1c96975
Compare
1c96975
to
afd79ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this!
Landed in fff4272 |
Ref: nodejs#17052 (comment) PR-URL: nodejs#17095 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Ref: #17052 (comment) PR-URL: #17095 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Ref: #17052 (comment) PR-URL: #17095 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Ref: #17052 (comment) PR-URL: #17095 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Ref: #17052 (comment) PR-URL: #17095 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Introduce sections in the Cpp style guide as suggested by @joyeecheung in
#17052 (comment).
Checklist
Affected core subsystem(s)
doc