From a4417a51eadd8435b61241d2b93e4357b175afaa Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 8 Oct 2017 12:25:19 -0700 Subject: [PATCH 1/6] doc: add basic C++ style guide Ideally, most of these things would be enforced via linter rules. This is a first step into having a style guide that goes beyond what the linter currently enforces. --- CONTRIBUTING.md | 3 ++ CPP_STYLE_GUIDE.md | 111 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+) create mode 100644 CPP_STYLE_GUIDE.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d78daedc802a13..2336deb7282285 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -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 diff --git a/CPP_STYLE_GUIDE.md b/CPP_STYLE_GUIDE.md new file mode 100644 index 00000000000000..4f7511dfe1d967 --- /dev/null +++ b/CPP_STYLE_GUIDE.md @@ -0,0 +1,111 @@ +# C++ style guide + +Unfortunately, the C++ linter (which can be run explicitly +via `make cpplint`) 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;` + +## 2 spaces of indentation for blocks or bodies of conditionals + +```c++ +if (foo) + bar(); +``` + +or + +```c++ +if (foo) { + bar(); + baz(); +} +``` + +(Braces are optional if the statement body only has one line.) + +## 4 spaces of indentation for statement continuations + +```c++ +VeryLongTypeName very_long_result = SomeValueWithAVeryLongName + + SomeOtherValueWithAVeryLongName; +``` + +## 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, + ...); +``` + +## CamelCase for methods, functions and classes + +```c++ +class FooBar { + public: + void DoSomething(); + static void DoSomethingButItsStaticInstead(); +}; +``` + +## 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 +class FancyContainer { + ... +} +``` + +## 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 + +## Memory allocation + +- `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 errors in nested C++ methods + +If you need to throw 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. From 39ab86738afbc293a50affe3352fa13eaab4b254 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 8 Oct 2017 14:02:39 -0700 Subject: [PATCH 2/6] address nits --- CPP_STYLE_GUIDE.md | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/CPP_STYLE_GUIDE.md b/CPP_STYLE_GUIDE.md index 4f7511dfe1d967..b4b15e1cc3fd9d 100644 --- a/CPP_STYLE_GUIDE.md +++ b/CPP_STYLE_GUIDE.md @@ -1,9 +1,10 @@ # C++ style guide -Unfortunately, the C++ linter (which can be run explicitly -via `make cpplint`) 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: +Unfortunately, the C++ linter (based on +[Google’s `cpplint`](https://github.com/google/styleguide)), which can be run +explicitly via `make cpplint`, 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 @@ -52,11 +53,17 @@ void FunctionWithAReallyReallyReallyLongNameSeriouslyStopIt( ## 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(); + + void set_foo_flag(int flag_value); + int foo_flag() const; // Use const-correctness whenever possible. }; ``` @@ -102,10 +109,12 @@ class FancyContainer { What it says in the title. -## Avoid throwing errors in nested C++ methods +## 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. -If you need to throw errors from a C++ binding method, try to do it at the -top level and not inside of nested calls. +(Using C++ `throw` is not allowed.) A lot of code inside Node.js is written so that typechecking etc. is performed in JavaScript. From b6a028ca1a391938fe836bc04af4059ad89dd651 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 8 Oct 2017 14:35:13 -0700 Subject: [PATCH 3/6] address more nits --- CPP_STYLE_GUIDE.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CPP_STYLE_GUIDE.md b/CPP_STYLE_GUIDE.md index b4b15e1cc3fd9d..2e3faedc16524b 100644 --- a/CPP_STYLE_GUIDE.md +++ b/CPP_STYLE_GUIDE.md @@ -67,7 +67,7 @@ class FooBar { }; ``` -## snake_case for local variables and parameters +## snake\_case for local variables and parameters ```c++ int FunctionThatDoesSomething(const char* important_string) { @@ -75,7 +75,7 @@ int FunctionThatDoesSomething(const char* important_string) { } ``` -## snake_case_ for private class fields +## snake\_case\_ for private class fields ```c++ class Foo { @@ -114,7 +114,7 @@ What it says in the title. 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. -(Using C++ `throw` is not allowed.) - A lot of code inside Node.js is written so that typechecking etc. is performed in JavaScript. + +(Using C++ `throw` is not allowed.) From 66a546d6ba36c8327c4317cfacd3508104ce3c85 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 10 Oct 2017 15:19:16 +0200 Subject: [PATCH 4/6] more nits --- CPP_STYLE_GUIDE.md | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/CPP_STYLE_GUIDE.md b/CPP_STYLE_GUIDE.md index 2e3faedc16524b..d8e188546d71a1 100644 --- a/CPP_STYLE_GUIDE.md +++ b/CPP_STYLE_GUIDE.md @@ -2,7 +2,7 @@ Unfortunately, the C++ linter (based on [Google’s `cpplint`](https://github.com/google/styleguide)), which can be run -explicitly via `make cpplint`, does not currently catch a lot of rules that are +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: @@ -28,6 +28,8 @@ if (foo) { (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++ @@ -35,6 +37,8 @@ VeryLongTypeName very_long_result = SomeValueWithAVeryLongName + SomeOtherValueWithAVeryLongName; ``` +Operators are before the line break in these cases. + ## Align function arguments vertically ```c++ @@ -51,6 +55,20 @@ void FunctionWithAReallyReallyReallyLongNameSeriouslyStopIt( ...); ``` +## Initialization lists + +Long initialization lists are formatted like this: + +```c++ +HandleWrap::HandleWrap(Environment* env, + Local 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 From 68176fe0d8dad14bf1c43fba7f963cc0558a0341 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 12 Oct 2017 02:34:07 +0200 Subject: [PATCH 5/6] [squash] cjihrig nits --- CPP_STYLE_GUIDE.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CPP_STYLE_GUIDE.md b/CPP_STYLE_GUIDE.md index d8e188546d71a1..277496f0d82eb8 100644 --- a/CPP_STYLE_GUIDE.md +++ b/CPP_STYLE_GUIDE.md @@ -26,7 +26,7 @@ if (foo) { } ``` -(Braces are optional if the statement body only has one line.) +Braces are optional if the statement body only has one line. `namespace`s receive no indentation on their own. @@ -135,4 +135,4 @@ 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.) +Using C++ `throw` is not allowed. From 8b289f153c7fc37db9696988aca5fcc71153508b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 13 Oct 2017 07:39:22 +0200 Subject: [PATCH 6/6] [squash] capitalize Style Guide --- CPP_STYLE_GUIDE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CPP_STYLE_GUIDE.md b/CPP_STYLE_GUIDE.md index 277496f0d82eb8..3e7319c1f8e8f7 100644 --- a/CPP_STYLE_GUIDE.md +++ b/CPP_STYLE_GUIDE.md @@ -1,4 +1,4 @@ -# C++ style guide +# C++ Style Guide Unfortunately, the C++ linter (based on [Google’s `cpplint`](https://github.com/google/styleguide)), which can be run