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: document nullptr comparisons in style guide #23805

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

This documents existing practices.

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

This documents existing practices.
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 21, 2018
@addaleax addaleax added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 21, 2018
@refack
Copy link
Contributor

refack commented Oct 21, 2018

I thought we agreed to adopt the C++ Core Guidelines exactly to close the omissions in the style guide, and have explicit and well reasoned guidelines.
So this is change the current guideline ES.87: Don’t add redundant == or != to conditions to the opposite.

Could you provide reasons for this change?

@addaleax
Copy link
Member Author

Could you provide reasons for this change?

What is says in the PR description. It documents what we currently have. Preferring to document existing style before working on changes was also what was echoed as a sentiment in the last TSC meeting.

I thought we agreed to adopt the C++ Core Guidelines exactly to close the omissions in the style guide, and have explicit and well reasoned guidelines.

We agreed to link these documents since they provide meaningful guidelines. They do not match our current style in all cases; for those cases, we have this very document.


Use explicit comparisons to `nullptr` when testing pointers, i.e.
`if (foo == nullptr)` instead of `if (foo)` and
`foo != nullptr` instead of `!foo`.
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, maybe make this if (foo != nullptr) and if (!foo).

Copy link
Member Author

Choose a reason for hiding this comment

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

@cjihrig yup, done!

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 24, 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.

I would like to ask for a reference for where this guideline came from, and what is it's reasoning.

@addaleax
Copy link
Member Author

@refack This is, as the PR says, an existing practice that we follow. I can only speculate about the reasons why it was introduced, but it does follow the general “explicit is better than implicit” rule, and makes clear whether a check tests a pointer or a boolean.

The origins of the rule definitely predate my time around here (and I’m not so sure about their relevance). @bnoordhuis might know.

@danbev
Copy link
Contributor

danbev commented Oct 25, 2018

Landed in 24cb1f3.

@danbev danbev closed this Oct 25, 2018
danbev pushed a commit that referenced this pull request Oct 25, 2018
This documents existing practices.

PR-URL: #23805
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
targos pushed a commit that referenced this pull request Oct 26, 2018
This documents existing practices.

PR-URL: #23805
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
targos pushed a commit that referenced this pull request Nov 1, 2018
This documents existing practices.

PR-URL: #23805
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
This documents existing practices.

PR-URL: #23805
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
This documents existing practices.

PR-URL: #23805
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
This documents existing practices.

PR-URL: #23805
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@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.

7 participants