FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: add basic C++ style guide by addaleax · Pull Request #16090 · nodejs/node · GitHub

/ node Public

doc: add basic C++ style guide - #16090

Closed
addaleax wants to merge 6 commits into
nodejs:masterfrom
addaleax:cpp-style-guide
Closed

doc: add basic C++ style guide#16090
addaleax wants to merge 6 commits into
nodejs:masterfrom
addaleax:cpp-style-guide

Conversation

addaleax commented Oct 8, 2017

Copy link
Copy Markdown
Member

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 added the doc Issues and PRs related to the documentations. label Oct 8, 2017
addaleax added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 8, 2017

bnoordhuis left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread CPP_STYLE_GUIDE.md Outdated

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread CPP_STYLE_GUIDE.md Outdated

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

refack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yay 👏

Comment thread CPP_STYLE_GUIDE.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread CPP_STYLE_GUIDE.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

👍 Cool. Examples beat all.

jasnell left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Awesome!

Comment thread CPP_STYLE_GUIDE.md Outdated

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please escape the underscores for non-GitHub markdown readers.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

thanks, done!

Comment thread CPP_STYLE_GUIDE.md Outdated

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member

cc @BridgeAR

TimothyGu left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Oct 8, 2017

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

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

Comment thread CPP_STYLE_GUIDE.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@danbev yes, I guess so :)

Fishrock123 left a comment
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

Comment thread CPP_STYLE_GUIDE.md Outdated

Fishrock123 Oct 9, 2017
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could these two be clarified... slightly?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Oct 9, 2017

Copy link
Copy Markdown
Member

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

trevnorris left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Doc looks great.

Left a few notes, but nothing blocking.

Comment thread CPP_STYLE_GUIDE.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread CPP_STYLE_GUIDE.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread CPP_STYLE_GUIDE.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

bnoordhuis Oct 10, 2017
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
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 commented Oct 10, 2017

Copy link
Copy Markdown
Member

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.

Comment thread CPP_STYLE_GUIDE.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The parens can probably be dropped here.

Comment thread CPP_STYLE_GUIDE.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Parens could be dropped here as well.

mhdawson left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member Author

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

refack commented Oct 12, 2017

Copy link
Copy Markdown
Contributor

Landing this PR will resolve #12636

Comment thread CPP_STYLE_GUIDE.md Outdated
@@ -0,0 +1,138 @@
# C++ style guide

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

done!

Comment thread CPP_STYLE_GUIDE.md

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

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

void* foo, * bar;

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'd just declare one variable per line

Comment thread CPP_STYLE_GUIDE.md

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

## 2 spaces of indentation for blocks or bodies of conditionals

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Landed in 23340b9

addaleax closed this Oct 14, 2017
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>

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.


Back | FazBrowse Home | New Git URL