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.

Sign up for GitHub

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

Jump to bottom

Remove redundant nullptr checks before deallocation #10696

Merged
merged 1 commit into from Nov 9, 2017
Merged

Remove redundant nullptr checks before deallocation #10696

merged 1 commit into from Nov 9, 2017

Conversation

practicalswift
Copy link
Contributor

Rationale:

  • delete ptr is a no-op if ptr is nullptr

@jonasschnelli
Copy link
Contributor

Is that completely safe?
AFAIK the C++03 standard kept deleting a nullptr (NULL) as undefined. Not sure about C++11.

@sipa
Copy link
Member

sipa commented Jun 29, 2017

According to https://stackoverflow.com/questions/25329576/calling-delete-on-null-pointers-c03-vs-c11, C++11 (accidentally?) dropped the requirement that deleting a nullptr is a noop, and C++14 is clearer about it again.

@practicalswift
Copy link
Contributor Author

practicalswift commented Jun 29, 2017 •
edited

Ouch, if I'm mistaken regarding delete NULL being a no-op in C++11. If that is the case I'll instead take a look at all delete:s in the code base to make sure that we are not accidentally doing any potential delete NULL:s.

@laanwj
Copy link
Member

laanwj commented Jun 30, 2017 •
edited

Ouch, if I'm mistaken regarding delete NULL being a no-op in C++11

We assume universally that delete NULL is a no-op. 99% of actual C++ code out there does so. This is just a language lawyer's argument.
No compiler will break that behavior. As it is resolved again for C++14, please don't plow over the entire source code to remove that assumption.

@practicalswift
Copy link
Contributor Author

practicalswift commented Jun 30, 2017 •
edited

@laanwj Yes, that was my assumption from the beginning :-) This PR should be completely safe then and consistent with the assumptions made in the rest of the code base with regards to delete NULL being a no-op.

@practicalswift
Remove redundant nullptr checks before deallocation
b109a1c 
Rationale:
* delete ptr is a no-op if ptr is nullptr
@practicalswift
Copy link
Contributor Author

Rebased!

@laanwj laanwj merged commit b109a1c into bitcoin:master Nov 9, 2017
laanwj added a commit that referenced this pull request Nov 9, 2017
@laanwj
Merge #10696: Remove redundant nullptr checks before deallocation
ef3758d 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
laanwj
laanwj reviewed Nov 9, 2017
View reviewed changes
src/net.cpp
@@ -2753,8 +2753,7 @@ CNode::~CNode()
{
CloseSocket(hSocket);

if (pfilter)
delete pfilter;
delete pfilter;
Copy link
Member

Choose a reason for hiding this comment

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

BTW as this one is a) in a destructor b) at the end of the destructor, why aren't we using a smart pointer w/ RAII?

@practicalswift
Copy link
Contributor Author

practicalswift commented Nov 9, 2017 •
edited

@laanwj Regarding pfilter RAII – that has already been taken care of in #11043 :-)

@TheBlueMatt
Copy link
Contributor

post-merge-obvious utACK b109a1c

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 22, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#10696: Remove redundant nullptr checks before deallocation
e7b1a8c 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 2, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10696: Remove redundant nullptr checks before deallocation
0a9e386 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 4, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10696: Remove redundant nullptr checks before deallocation
f3fc91a 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 12, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10696: Remove redundant nullptr checks before deallocation
da6741b 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 12, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10696: Remove redundant nullptr checks before deallocation
5b08b19 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 12, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10696: Remove redundant nullptr checks before deallocation
86a6a42 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 12, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10696: Remove redundant nullptr checks before deallocation
660d380 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 12, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10696: Remove redundant nullptr checks before deallocation
e932a56 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 12, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10696: Remove redundant nullptr checks before deallocation
4eb4845 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 16, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10696: Remove redundant nullptr checks before deallocation
bfa7f69 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10696: Remove redundant nullptr checks before deallocation
3bae57a 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10696: Remove redundant nullptr checks before deallocation
2ab2efc 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#10696: Remove redundant nullptr checks before deallocation
22cbe2c 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
@practicalswift practicalswift deleted the delete-nullptr branch April 10, 2021 19:32
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 25, 2021
@laanwj
Merge bitcoin#10696: Remove redundant nullptr checks before deallocation
a940e66 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jan 30, 2022
@laanwj
Merge bitcoin#10696: Remove redundant nullptr checks before deallocation
63e0000 
b109a1c Remove redundant nullptr checks before deallocation (practicalswift)

Pull request description:

  Rationale:
  * `delete ptr` is a no-op if `ptr` is `nullptr`

Tree-SHA512: c98ce769125c4912186a8403cc08a59cfba85b7141af645c709b4c4eb90dd9cbdd6ed8076d50099d1e4ec2bf75917d1af6844082ec42bbb4d94d229a710e051c
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@practicalswift @jonasschnelli @sipa @laanwj @TheBlueMatt @fanquake