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

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Oct 8, 2017

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.

This is probably relevant to most of: @bnoordhuis @jasnell @TimothyGu @trevnorris @danbev @eugeneo @nodejs/n-api

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 8, 2017
@addaleax addaleax added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 8, 2017
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with two suggestions. Good idea and nice work, Anna!

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.)


What it says in the title.

## Avoid throwing errors in nested C++ methods
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: s/errors/JavaScript errors/ - this section could be interpreted to mean that throw is allowed.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Yay 👏

@@ -0,0 +1,111 @@
# C++ style guide

Unfortunately, the C++ linter (which can be run explicitly
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO Google's style guide is still a good reference.

...the C++ linter (based on [Google's `cpplint`](https://github.com/google/styleguide), and which can...

...);
```

## CamelCase for methods, functions and classes
Copy link
Contributor

Choose a reason for hiding this comment

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

When I grew up this was called PascalCase, and camel case is what's now known as lowerCamelCase

Copy link
Member Author

Choose a reason for hiding this comment

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

I’ve never heard of that term, and in any case the example’s very purpose is to avoid any ambiguity about what is meant.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Cool. Examples beat all.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Awesome!

};
```

## snake_case for local variables and parameters
Copy link
Member

Choose a reason for hiding this comment

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

Please escape the underscores for non-GitHub markdown readers.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, done!

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.)
Copy link
Member

Choose a reason for hiding this comment

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

This sentence sounds odd being positioned here, sandwiched between two sentences talking about JavaScript errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

I’ve moved it to the end. I added it because it seemed to make sense to me that, like @bnoordhuis said, there might be confusion about what “throwing” means, but I’d also be happy just dropping it

@TimothyGu
Copy link
Member

cc @BridgeAR

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

There's also:

  • namespace does not increase indentation level
  • Wrap at 80 cols
  • Use references (char&) only if it's constant (const char&); otherwise use pointers (char*)

@addaleax
Copy link
Member Author

addaleax commented Oct 8, 2017

@TimothyGu This was specifically about things that the linter doesn’t complain about, I’m pretty sure it does for all of these things

@TimothyGu
Copy link
Member

@addaleax It doesn't always report the first one (see ade80ee). The rest, yes, I believe cpplint does report them all.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this perhaps be make lint-cpp instead? Running make cppilnt target produces the following warning:

$ make cpplint
Running C++ linter...
Total errors found: 0
Please use lint-cpp instead of cpplint

Copy link
Member Author

Choose a reason for hiding this comment

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

@danbev yes, I guess so :)

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

FunctionWithAReallyReallyReallyLongNameSeriouslyStopIt 😂

Mind to punctuate the lists?

Aside form the memory section comment though this is very helpful!

Should this doc be top-level, or live inside doc/? I suppose we may want it visible and then just get rid of it once we get some sort of new linting in place?

- Use `static_cast` for casting whenever it works
- `reinterpret_cast` is okay if `static_cast` is not appropriate

## 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.

@gibfahn
Copy link
Member

gibfahn commented Oct 9, 2017

Should this doc be top-level, or live inside doc/? I suppose we may want it visible and then just get rid of it once we get some sort of new linting in place?

My first thought would be to put it in doc with the other STYLE_GUIDE.

It might be worth including a reference to it in PULL_REQUEST_TEMPLATE.md too, but on the other hand that template is already quite long already, and lots of people don't read it (and it wouldn't apply to most PRs).

Copy link
Contributor

@trevnorris trevnorris left a comment

Choose a reason for hiding this comment

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

Doc looks great.

Left a few notes, but nothing blocking.

...
}
```

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.

- 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


```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.

@addaleax
Copy link
Member Author

@TimothyGu

namespace does not increase indentation level

I’ve added a sentence for this.

@gibfahn

My first thought would be to put it in doc with the other STYLE_GUIDE.

That’s in doc/ because it’s a style guide for documentation, though.

@gibfahn
Copy link
Member

gibfahn commented Oct 10, 2017

That’s in doc/ because it’s a style guide for documentation, though.

Then maybe in src/? Should show up at the top in there. I think that the top level directory is pretty cluttered as it is, and I wonder if putting it in a sub-directory might make it more discoverable. I've been told by a few people that the top level directory has too much, and it's difficult to find anything.

Non-blocking though, feel free to ignore if you disagree.

}
```

(Braces are optional if the statement body only has one line.)
Copy link
Contributor

Choose a reason for hiding this comment

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

The parens can probably be dropped here.

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

(Using C++ `throw` is not allowed.)
Copy link
Contributor

Choose a reason for hiding this comment

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

Parens could be dropped here as well.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM, great to have this guidance written down,

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.
@addaleax
Copy link
Member Author

@gibfahn I’m still a bit worried that that would make this a lot less easily discoverable…

@refack
Copy link
Contributor

refack commented Oct 12, 2017

Landing this PR will resolve #12636

@@ -0,0 +1,138 @@
# C++ style guide
Copy link
Member

Choose a reason for hiding this comment

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

This is capitalized in CONTRIBUTING.md, I'd suggest to do the same here.

Copy link
Member Author

Choose a reason for hiding this comment

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

done!


## 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


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

## 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.

@addaleax
Copy link
Member Author

Landed in 23340b9

@addaleax addaleax closed this Oct 14, 2017
@addaleax addaleax deleted the cpp-style-guide branch October 14, 2017 09:14
addaleax added a commit that referenced this pull request Oct 14, 2017
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.

PR-URL: #16090
Fixes: #12636
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
addaleax added a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
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.

PR-URL: nodejs/node#16090
Fixes: nodejs/node#12636
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit that referenced this pull request Oct 18, 2017
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.

PR-URL: #16090
Fixes: #12636
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.