Skip to content

Remove checks for nullptrs before deleting, since deleting is a no-op#1615

Open
jmcarcell wants to merge 1 commit intoAIDASoft:masterfrom
jmcarcell:delete-checks
Open

Remove checks for nullptrs before deleting, since deleting is a no-op#1615
jmcarcell wants to merge 1 commit intoAIDASoft:masterfrom
jmcarcell:delete-checks

Conversation

@jmcarcell
Copy link
Copy Markdown
Member

BEGINRELEASENOTES

  • Remove checks for nullptrs before deleting, since deleting is a no-op

ENDRELEASENOTES

@MarkusFrankATcernch
Copy link
Copy Markdown
Contributor

NO.
This is no good! Double delete leads to SEGV.
Maybe delete (nullptr); is a noop, but the change at line 340 is a pure desaster.

@jmcarcell
Copy link
Copy Markdown
Member Author

But there is no double deletion.

  • Before this PR: Only gets deleted when not nullptr
  • Now: Always gets deleted, but delete nullptr doesn't do anything, so nothing changes.
    All tests are passing, if there was double deletion some tests would not pass since these are used in many places.

@MarkusFrankATcernch
Copy link
Copy Markdown
Contributor

@jmcarcell Not true. the deletion at line 340 does unconditionally delete whatever is at the pointer.
The fact that it just not happens now is pure luck. deletePtr resets the external pointer to null. deleteObject does not. Hence it could be called twice outside of the control of the function.

@MarkusFrankATcernch
Copy link
Copy Markdown
Contributor

Another point is: we do not have to optimize to the last instruction.
Geometry and detector description by nature are rather static.
Better play always safe.

@github-actions
Copy link
Copy Markdown

Test Results

   18 files     18 suites   6h 17m 41s ⏱️
  357 tests   357 ✅ 0 💤 0 ❌
3 143 runs  3 143 ✅ 0 💤 0 ❌

Results for commit 68db601.

@jmcarcell
Copy link
Copy Markdown
Member Author

@jmcarcell Not true. the deletion at line 340 does unconditionally delete whatever is at the pointer. The fact that it just not happens now is pure luck. deletePtr resets the external pointer to null. deleteObject does not. Hence it could be called twice outside of the control of the function.

True if called twice there will be a double delete for a non-null ptr, but this is true with the changes in this PR and without them. The changes in the PR do not change the behaviour that one sees for both a nullptr and a non-null ptr in both functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants