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: formalize non-const reference usage in C++ style guide #23155

Closed

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Sep 29, 2018

Split out from #23028 by request from @refack. I’ve added people who approved the other PR as reviewers on the commit. I’m also moving the tsc-agenda label over here, assuming @refack’s objection stands.


We generally avoid using non-const references if not necessary. This
formalizes this rules by writing them down in the C++ style guide.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

We generally avoid using non-const references if not necessary. This
formalizes this rules by writing them down in the C++ style guide.

Refs: nodejs#23028
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@addaleax addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations. tsc-agenda Issues and PRs to discuss during the meetings of the TSC. labels Sep 29, 2018
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.

Hoping we can discuss this further, and find middle ground.

@refack
Copy link
Contributor

refack commented Sep 29, 2018

Quoting from previous PR:

I believe non-const reference has been established as the tool to use for idiomatic iterative mutation in "modern" C++ (C++11 and higher).

I'm taking the lead of the C++ ISO committee and the STL designers who decided to use references, and value semantics ahead of pointers. Case in point begin and end return iterators that are convertible to references and not pointers. So to mutate a collection with an STL algorithm, you can only use non-const references:

std::vector<int> nums{3, 4, 2, 8, 15, 267};
std::for_each(nums.begin(), nums.end(), [](int &n){ n++; });

From the C++CG:

...

for (const auto& x : v) { /* do something with the value of x */ }

Now, there is no explicit mention of the iteration mechanism, and the loop operates on a reference to const elements so that accidental modification cannot happen. If modification is desired, say so:

for (auto& x : v) { /* modify x */ }

With code that came up in a code review in our repo, it was presented with a common classical way to do mutation without a reference:

for (std::string::size_type i = 0; i < name.size(); ++i) {
  if (name[i] == '_') 
    name[i] = '-';
}

Compared this with the C++11 idiomatic (read "this is iterating using a reference because the intent is to modify").

for (auto& i : name) {
  if (i == '_') 
    i = '-';
}

So essentially IMHO having "no non-const references" rule, means "no idiomatic mutations".

From a personal perspective, I also agree with a few more arguments in favor of non-const reference:

  1. They can't be uninitialized.
  2. They can't be null, so they fit perfectly to the use case of a variable is not allowed to be null.
  3. They clearly express non-ownership, while with pointers there is still doubt.
  4. They provide the compiler with more optimization opportunities (unless the compiler can to prove you don't need a pointer, it will have to allocate space for it).
  5. IMHO they are simpler to understand. Grokking pointers requires always keeping another level of indirection in mind.

BTW, both guides do have limits, spesificly WRT to function arguments:

@refack
Copy link
Contributor

refack commented Sep 29, 2018

P.S. Using as more standard guidelines will allow us to introduce automatic tools (MSVS and clang-tidy) to analyse, and point out non conforming code.

@targos
Copy link
Member

targos commented Sep 29, 2018

@refack would you be for making the rule more specific? i.e. only avoid non-const references in function parameters?

@refack
Copy link
Contributor

refack commented Sep 29, 2018

P.S. I forget to mention the thing that is most important to me: learning material.
All the materials I've encountered use non-const references, and explain their idiomatic meaning. (Just a quick example of something I just read 1).

IMHO it is immensely important for our code and guidance to be in line with the available learning material, to allow the inclusion of newcomers (or relearning late comers like myself)

@refack
Copy link
Contributor

refack commented Sep 29, 2018

would you be for making the rule more specific? i.e. only avoid non-const references in function parameters

Yes. Something like "non const reference function argument means in/out parameter. Avoid in/out parameters" reads very clear and sound to me.

@addaleax
Copy link
Member Author

would you be for making the rule more specific? i.e. only avoid non-const references in function parameters

Yes. Something like "non const reference function argument means in/out parameter. Avoid in/out parameters" reads very clear and sound to me.

I’m heavily -1 on that. For in/out, we should keep using pointers, since it’s otherwise completely invisible at the call site that it’s an in/out parameter.

From a personal perspective, I also agree with a few more arguments in favor of non-const reference:

  1. They can't be uninitialized.

That’s true, yes – It’s a feature that pointers lack.

  1. They can't be null, so they fit perfectly to the use case of a variable is not allowed to be null.

I don’t understand this – they are references, not variables themselves in the classical sense? Whatever the reference refers to could still be some kind of null/default value.

  1. They clearly express non-ownership, while with pointers there is still doubt.

That’s fair, although we are slowly moving a lot of our C++ code to use smart pointers, which means that on the reverse side regular pointers would typically be non-owning pointers.

  1. They provide the compiler with more optimization opportunities (unless the compiler can to prove you don't need a pointer, it will have to allocate space for it).

I doubt that, esp. the “have to allocate space for it”. Under the hood, references are pointers, and code using one type should always compile to the same assembly as code using the other type. Can you provide a reference for this claim?

  1. IMHO they are simpler to understand. Grokking pointers requires always keeping another level of indirection in mind.

Hm – not sure I can judge this well myself, but I’m surprised by that since they are essentially the same concept, up to minor differences (no arrays using references, no default value).

@refack It’s not that I don’t understand or don’t see your points – I have used references in my C++ code a lot more before coming to the Node.js world, and had to get used to the change myself. I fully acknowledge that that took a bit of learning when writing C++ code (with a lot of other things as well, such as variable/function naming conventions etc.).

However, I’m also carrying my reviewer hat a lot of the time, and since Node.js is a mature software which millions of people use in their daily lives, I tend to value readability and more easily visible correctness higher than the ease of writing code. That’s a tradeoff, and it’s fair to stand on either side of it.

Compared this with the C++11 idiomatic (read "this is iterating using a reference because the intent is to modify").

for (auto& i : name) {
  if (i == '_') 
    i = '-';
}

Note that I’m not saying that I’m for categorically excluding these types of constructs. However, I still hold my opinion that this code obscures the fact that name is being modified, and that’s a good example of what we’re trying to avoid.

@addaleax
Copy link
Member Author

@nodejs/tsc PTAL


private:
std::string foo_string_;
int* pointer_to_integer_; // Pointer instead of reference.
Copy link
Member

@joyeecheung joyeecheung Oct 1, 2018

Choose a reason for hiding this comment

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

Personally in cases like this I'd prefer to use a normal int here and return references because it's more memory-efficient (and if this is storing a pointer to mutate it the ownership seems to be pretty hard to figure out..)...maybe a member object with a made-up class here would be clearer? Or a unique_ptr since that's what we generally try to refactor to? (Or a shared_ptr but we don't seem to use it much in our codebase and try to restructure the ownership towards a unique_ptr)

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 … How about e9f8406?

Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

LGTM.

See also additional rationale in https://google.github.io/styleguide/cppguide.html#Reference_Arguments, which is followed by V8 and Chromium.

@Trott
Copy link
Member

Trott commented Oct 2, 2018

It would seem like the reason for applying the tsc-agenda label is to request a vote to override the objection. I'm not sure if there's more to discuss here first or if it's time to have a vote. If and when vote time comes, this is outside my area of expertise so I abstain and defer to other knowledgable TSC folks.

If we're voting, here's how it looks so far:

YES:

  1. @addaleax
  2. @apapirovski
  3. @danbev
  4. @jasnell
  5. @joyeecheung
  6. @ofrobots
  7. @targos

NO:
No one yet

ABSTAIN:

  1. @cjihrig
  2. @gabrielschulhof
  3. @mcollina
  4. @MylesBorins
  5. @rvagg
  6. @Trott

UPDATE: This passes. (19 TSC members w/ 6 abstentions = 7 votes required to pass.)


private:
std::string foo_string_;
// Pointer instead of reference. If this objects 'owns' the other object,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: object

Copy link
Member Author

Choose a reason for hiding this comment

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

@thefourtheye Thanks for catching, done!

private:
std::string foo_string_;
// Pointer instead of reference. If this objects 'owns' the other object,
// this should be be a `std::unique_ptr<OtherClass>`; a
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: double be

@mcollina
Copy link
Member

mcollina commented Oct 2, 2018

I abstain.

@MylesBorins
Copy link
Contributor

I abstain

@refack
Copy link
Contributor

refack commented Oct 16, 2018

@addaleax can we converge on "no non-const references as arguments in APIs"?


Avoid defining APIs with non-const reference arguments

In general, non-const references should only be used when they don't hurt readability, or if they are required by external APIs (e.g. std::for_each)

Non-const references arguments make most sense as in/out parameters [F.11]. The in/out parameter pattern is non-optimal, and should be avoided. If the parameter is a regular (in) parameter, it should be const. If in/out semantics is necessary, use a pointer as per the Google style guide.

@addaleax
Copy link
Member Author

@refack But this isn’t just about functions/methods…

Non-const references arguments make most sense as in/out parameters [F.11]. The in/out parameter pattern is non-optimal, and should be avoided. If the parameter is a regular (in) parameter, it should be const. If in/out semantics is necessary, use a pointer as per the Google style guide.

This seems a bit confusing – why would we first mention that one thing makes sense, and then that we don’t do that because the Google style guide overrides it?

@refack
Copy link
Contributor

refack commented Oct 16, 2018

But this isn’t just about functions/methods…

I'm looking for some compromise on your part...
Let's document the parts that have consensus.

@addaleax
Copy link
Member Author

@refack I’d prefer to avoid having this exact same discussion again about the other parts, though.

Do you agree with the examples in this PR?

@refack
Copy link
Contributor

refack commented Oct 17, 2018

That's why I asked for a compromise...

I don't see anything in the examples code that conflicts with the paragraph I suggested. I don't agree with the comments and the generalization that "A pointer is almost always a better choice."
AFAICT Modern C++ moves away from the classical C paradigm that "pointers are always better". And as I see it pointers and references are two equally valuable language constract that each have their own idioms, and should be used when appropriate.

@addaleax
Copy link
Member Author

That's why I asked for a compromise...

As far as I can tell, that comment asked for documenting this in the style guide for only a subset of the use cases – That’s not really what a compromise is. (And, to mention it again: This PR documents current, existing rules.)

I don't see anything in the examples code that conflicts with the paragraph I suggested.

Yes, but your suggestion would leave the question open for a wide range of use cases (e.g. local reference variables). Those cases would otherwise be pointed out during reviews, which is less than helpful if we can also document them in our style guide for.

And as I see it pointers and references are two equally valuable language constract that each have their own idioms, and should be used when appropriate.

They both have advantages and disadvantages, so, yes, of course this depends on the use case. The big disadvantage of references, and how that can be mitigated, is mentioned here in the PR text.

If you want, I can add something like “If you use non-const references, e.g. because of an external API that uses them, consider adding a comment that indicates which objects can be modified through them.”

@sam-github
Copy link
Contributor

@addaleax and @refack (and most of the c++ world?) agree that non-const ref args are to be avoided. @addaleax had a short paragraph elaborating why, @refack had a longer more detailed paragraph covering the same territory. More detail doesn't seem bad. I genuinely don't understand what's going on here.

"If in/out semantics is required, it is advised to use a pointer instead" and "a pointer is almost always a better choice" say the same thing, since in/out semantics is almost always why a ref arg is not const.

@addaleax
Copy link
Member Author

@sam-github The difference here is that @refack’s text only refers to function arguments, while we currently have this as a more general rule that also applies to e.g. local variables or class members.

@sam-github
Copy link
Contributor

Reviewed the comment thread again, and I don't see that difference in opinion. The only example Rafael showed of sane non-const refs was a for loop, which you have an example of in your code and also list as a reasonable use. If you amend your text to explicitly list the ways in which non-const refs are sometimes used, and which ones are OK (not many), and what they should be replaced with I think it would be more useful to readers of the style guide. Maybe you would disagree on some specific usage, but I don't see any specific disagreements in this thread (other than on how to express the guidance in english text).

@refack
Copy link
Contributor

refack commented Oct 17, 2018

(And, to mention it again: This PR documents current, existing rules.)

Since this rule is more lore then cannon (in that it is not written down nor linted for), and since C++11 is a modernization of the language. I see this as an excellent chance to improve our rules set. Or more correctly not canonize an outdated rule.

@refack
Copy link
Contributor

refack commented Oct 17, 2018

Even V8 deviate from this rule (listing points just from the V8 public API):

node/deps/v8/include/v8.h

Lines 3398 to 3399 in c1b9be5

V8_WARN_UNUSED_RESULT Maybe<bool> DefineProperty(
Local<Context> context, Local<Name> key, PropertyDescriptor& descriptor);

@addaleax
Copy link
Member Author

Even V8 deviate from this rule (listing points just from the V8 public API):

… which is why they explicitly mention this in the doc comment.

@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 18, 2018
@refack
Copy link
Contributor

refack commented Oct 18, 2018

@addaleax addaleax removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Oct 21, 2018
@addaleax
Copy link
Member Author

Landed in 5550510

@addaleax addaleax closed this Oct 21, 2018
@addaleax addaleax deleted the cpp-style-guide-no-non-const-references branch October 21, 2018 03:04
addaleax added a commit that referenced this pull request Oct 21, 2018
We generally avoid using non-const references if not necessary. This
formalizes this rules by writing them down in the C++ style guide.

(Note: Some reviews are from the original PR.)

Refs: #23028
PR-URL: #23155
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 21, 2018
We generally avoid using non-const references if not necessary. This
formalizes this rules by writing them down in the C++ style guide.

(Note: Some reviews are from the original PR.)

Refs: #23028
PR-URL: #23155
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
We generally avoid using non-const references if not necessary. This
formalizes this rules by writing them down in the C++ style guide.

(Note: Some reviews are from the original PR.)

Refs: #23028
PR-URL: #23155
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
We generally avoid using non-const references if not necessary. This
formalizes this rules by writing them down in the C++ style guide.

(Note: Some reviews are from the original PR.)

Refs: #23028
PR-URL: #23155
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
We generally avoid using non-const references if not necessary. This
formalizes this rules by writing them down in the C++ style guide.

(Note: Some reviews are from the original PR.)

Refs: #23028
PR-URL: #23155
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.