Skip to content

doc: make C++ core guidelines a reference#24315

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.

Sign up for GitHub

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

Closed
joyeecheung wants to merge 1 commit into from
Closed

doc: make C++ core guidelines a reference #24315

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions CPP_STYLE_GUIDE.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -38,19 +38,24 @@ runtime features.
Coding guidelines are based on the following guides (highest priority first):
1. This document
2. The [Google C++ Style Guide][]
3. The ISO [C++ Core Guidelines][]

In general code should follow the C++ Core Guidelines, unless overridden by the
Google C++ Style Guide or this document. At the moment these guidelines are
checked manually by reviewers, with the goal to validate this with automatic
tools.
In general code should the Google C++ Style Guide, unless overridden by this

@TrottTrottNov 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

typo? should the Google?

@TrottTrottNov 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(I guess follow is missing in the sentence?)

@TrottTrottNov 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd drop In general if possible. It serves only to make following the style guide seem optional. Even if it is optional, should seems to provide sufficient wiggle-room for those times when you can't follow it.

document.

## Formatting
There are two automatic tools in this code base:

- `make lint-cpp`: the C++ linter based on [Google’s `cpplint`][]
- `make format-cpp`: the C++ formatter based on clang-format

At the moment guidelines that cannot be caught by these tools are checked
manually by reviewers. When nit-picking about rules that cannot be enforced
by automatic tooling during review, prefer giving specific reasons on a
case-by-case basis instead of simply citing rules.

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:
In code that does not interface with any third-party libraries,
[C++ Core Guidelines][] may be a reference for good practices.

@TrottTrottNov 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

may be -> are?


## Formatting

### Left-leaning (C++ style) asterisks for pointer declarations

Expand Down