Skip to content
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: add basic C++ style guide #16090

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,9 @@ should follow the [Style Guide](doc/STYLE_GUIDE.md). Code samples included
in the API docs will also be checked when running `make lint` (or
`vcbuild.bat lint` on Windows).

For contributing C++ code, you may want to look at the
[C++ Style Guide](CPP_STYLE_GUIDE.md).

#### Step 4: Commit

It is a recommended best practice to keep your changes as logically grouped
Expand Down
138 changes: 138 additions & 0 deletions CPP_STYLE_GUIDE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
# C++ Style Guide

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:

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

`char* buffer;` instead of `char *buffer;`
Copy link
Member

Choose a reason for hiding this comment

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

No doubt this is correct, but it always bothers me with multiple variables:

void* foo, * bar;

Copy link
Member

Choose a reason for hiding this comment

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

I'd just declare one variable per line


## 2 spaces of indentation for blocks or bodies of conditionals
Copy link
Member

Choose a reason for hiding this comment

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

Also for loops (unless conditionals includes loops).

while (something)
  ChangeSomething();

Now that I am thinking about it, I am not sure we use loops without parens...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, conditionals includes conditional loops. :) Anyway, I don’t think the current phrasing leaves anybody thinking that we use indentation other than 2 spaces.


```c++
if (foo)
bar();
```

or

```c++
if (foo) {
bar();
baz();
}
```

Braces are optional if the statement body only has one line.

`namespace`s receive no indentation on their own.

## 4 spaces of indentation for statement continuations

```c++
VeryLongTypeName very_long_result = SomeValueWithAVeryLongName +
SomeOtherValueWithAVeryLongName;
Copy link
Contributor

Choose a reason for hiding this comment

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

the linter doesn't currently catch whether the operation is at the end of the line or the beginning of the next line. couple examples:

// ternary
int r = true ?
    1 : 0;
int r = true
    ? 1 : 0;

// conditional
if (foo &&
    bar) { }
if (foo
    && bar) { }

specify this?

@addaleax /cc @bnoordhuis

Copy link
Member

@bnoordhuis bnoordhuis Oct 10, 2017

Choose a reason for hiding this comment

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

The && should go on the same line as the expression preceding it. (edit: but I believe the linter already complains about that.)

Ternary operator: we don't have a hard rule, I think, but IMO if it doesn't fit on a single line, you should be using an if statement anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

re: placement of &&. linter doesn't check that. should be possible to add to the linter? if so then no reason to put it here.

```

Operators are before the line break in these cases.

## Align function arguments vertically

```c++
void FunctionWithAVeryLongName(int parameter_with_a_very_long_name,
double other_parameter_with_a_very_long_name,
...);
```

If that doesn’t work, break after the `(` and use 4 spaces of indentation:

```c++
void FunctionWithAReallyReallyReallyLongNameSeriouslyStopIt(
int okay_there_is_no_space_left_in_the_previous_line,
...);
```

## Initialization lists

Long initialization lists are formatted like this:

```c++
HandleWrap::HandleWrap(Environment* env,
Local<Object> object,
uv_handle_t* handle,
AsyncWrap::ProviderType provider)
: AsyncWrap(env, object, provider),
state_(kInitialized),
handle_(handle) {
```

## CamelCase for methods, functions and classes

Exceptions are simple getters/setters, which are named `property_name()` and
`set_property_name()`, respectively.

```c++
class FooBar {
public:
void DoSomething();
static void DoSomethingButItsStaticInstead();
Copy link
Member

Choose a reason for hiding this comment

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

We also use foo() and set_foo() for simple getters/setters (with const-correctness whenever possible.)


void set_foo_flag(int flag_value);
int foo_flag() const; // Use const-correctness whenever possible.
};
```

## snake\_case for local variables and parameters

```c++
int FunctionThatDoesSomething(const char* important_string) {
const char* pointer_into_string = important_string;
}
```

## snake\_case\_ for private class fields

```c++
class Foo {
private:
int counter_ = 0;
};
```

## Space after `template`

```c++
template <typename T>
class FancyContainer {
...
}
```

Copy link
Contributor

Choose a reason for hiding this comment

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

another common pattern is how we handle indentation of initialization lists. example:

HandleWrap::HandleWrap(Environment* env,
                       Local<Object> object,
                       uv_handle_t* handle,
                       AsyncWrap::ProviderType provider)
    : AsyncWrap(env, object, provider),
      state_(kInitialized),
      handle_(handle) {

basically: if the initialization list does not fit on one line then it returns to the next line, indents 4 spaces then starts with the colon. Every additional member in the list starts on a new line and indented 6 spaced, to align with the first member in the list.

## Type casting

- Always avoid C-style casts (`(type)value`)
- `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
Copy link
Contributor

Choose a reason for hiding this comment

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

it may be worth mentioning usage of const_cast (since it's used in core) but not sure if there's a solid rule of when it's acceptable.

Copy link
Member

Choose a reason for hiding this comment

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

const_cast: should be used sparingly and only when it's still conceptually const afterwards.

Copy link
Member Author

Choose a reason for hiding this comment

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

there is nothing specific to Node about using const_cast, anything we’d write down here would apply to any other C++ code as well


## Memory allocation
Copy link
Contributor

@Fishrock123 Fishrock123 Oct 9, 2017

Choose a reason for hiding this comment

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

Could these two be clarified... slightly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have any specific question? I realize this isn’t helpful when it comes to which-do-I-use, but then again we don’t really follow any specific rules for this right now. (I assume @bnoordhuis is a fan of just aborting in OOM situations, me not so much. 😄)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, ok. On re-reading it I think I understand, maybe we could clarify the wording like so:

- Use `Malloc()`, `Calloc()`, etc. from `util.h` to cause an abort in Out-of-Memory situations.
- Use `UncheckedMalloc()`, etc. to return a `nullptr` in OOM situations.


- `Malloc()`, `Calloc()`, etc. from `util.h` abort in Out-of-Memory situations
- `UncheckedMalloc()`, etc. return `nullptr` in OOM situations

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

What it says in the title.

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

If you need to throw JavaScript errors from a C++ binding method, try to do it
at the top level and not inside of nested calls.

A lot of code inside Node.js is written so that typechecking etc. is performed
in JavaScript.

Using C++ `throw` is not allowed.