| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
LGTM with two suggestions. Good idea and nice work, Anna!
Sorry, something went wrong.
There was a problem hiding this comment.
We also use foo() and set_foo() for simple getters/setters (with const-correctness whenever possible.)
Sorry, something went wrong.
There was a problem hiding this comment.
Suggestion: s/errors/JavaScript errors/ - this section could be interpreted to mean that throw is allowed.
Sorry, something went wrong.
There was a problem hiding this comment.
Yay 👏
Sorry, something went wrong.
There was a problem hiding this comment.
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...
Sorry, something went wrong.
There was a problem hiding this comment.
When I grew up this was called PascalCase, and camel case is what's now known as lowerCamelCase
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
👍 Cool. Examples beat all.
Sorry, something went wrong.
There was a problem hiding this comment.
Awesome!
Sorry, something went wrong.
There was a problem hiding this comment.
Please escape the underscores for non-GitHub markdown readers.
Sorry, something went wrong.
There was a problem hiding this comment.
thanks, done!
Sorry, something went wrong.
There was a problem hiding this comment.
This sentence sounds odd being positioned here, sandwiched between two sentences talking about JavaScript errors.
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
There was a problem hiding this comment.
There's also:
Sorry, something went wrong.
|
@TimothyGu This was specifically about things that the linter doesn’t complain about, I’m pretty sure it does for all of these things |
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
There was a problem hiding this comment.
@danbev yes, I guess so :)
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
There was a problem hiding this comment.
Could these two be clarified... slightly?
Sorry, something went wrong.
There was a problem hiding this comment.
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. 😄)
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
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). |
Sorry, something went wrong.
There was a problem hiding this comment.
Doc looks great.
Left a few notes, but nothing blocking.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
const_cast: should be used sparingly and only when it's still conceptually const afterwards.
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
I’ve added a sentence for this.
That’s in doc/ because it’s a style guide for documentation, though. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
The parens can probably be dropped here.
Sorry, something went wrong.
There was a problem hiding this comment.
Parens could be dropped here as well.
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM, great to have this guidance written down,
Sorry, something went wrong.
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.
|
@gibfahn I’m still a bit worried that that would make this a lot less easily discoverable… |
Sorry, something went wrong.
|
Landing this PR will resolve #12636 |
Sorry, something went wrong.
| @@ -0,0 +1,138 @@ | |||
| # C++ style guide | |||
There was a problem hiding this comment.
This is capitalized in CONTRIBUTING.md, I'd suggest to do the same here.
Sorry, something went wrong.
There was a problem hiding this comment.
done!
Sorry, something went wrong.
|
|
||
| ## Left-leaning (C++ style) asterisks for pointer declarations | ||
|
|
||
| `char* buffer;` instead of `char *buffer;` |
There was a problem hiding this comment.
No doubt this is correct, but it always bothers me with multiple variables:
void* foo, * bar;
Sorry, something went wrong.
There was a problem hiding this comment.
I'd just declare one variable per line
Sorry, something went wrong.
|
|
||
| `char* buffer;` instead of `char *buffer;` | ||
|
|
||
| ## 2 spaces of indentation for blocks or bodies of conditionals |
There was a problem hiding this comment.
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...
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah, conditionals includes conditional loops. :) Anyway, I don’t think the current phrasing leaves anybody thinking that we use indentation other than 2 spaces.
Sorry, something went wrong.
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>
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>
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>
|
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 |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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