Repository navigation
Refactor to use std::unique_ptr instead of naked pointers - #9281
Conversation
Since delete just marks the memory as "being free for reuse" and lhost does not immediately go out of scope and is used in several places it is safer to set it to nullptr following the delete
nksanthosh
changed the title
| if (lhost) { | ||
| bindings.emplace_back(move(*lhost)); | ||
| delete lhost; | ||
| lhost = nullptr; |
There was a problem hiding this comment.
I think it would be better to change this to use a std::optional
There was a problem hiding this comment.
Decided to make it a std::unique_ptr.
| } // for i | ||
| bindings.emplace_back( move(*lhost) ); | ||
| delete lhost; | ||
| lhost = nullptr; |
There was a problem hiding this comment.
Since this is a local variable, I don't think this is needed, but if we make this change, I think for maintainability we should instead make lhost a unique_ptr and change line 810 and 851 to be "bindings.push_back(*lhost);" so we don't have to keep track of memory. This use case will not loop through this enough times for the moving of memory to make a significant impact.
There was a problem hiding this comment.
Brian - please take a look at the revised implementation. Thanks.
nksanthosh
changed the title
Since delete just marks the memory as "being free for reuse" and lhost does not immediately go out of scope and is used in several places it is safer to use std::unique_ptr instead of naked pointers and the corresponding delete
johndebord
left a comment
johndebord
left a comment
There was a problem hiding this comment.
LGTM. reset is called under the hood for the assignment operator; so it looks safe to me.
https://stackoverflow.com/questions/16061407/should-i-assign-or-reset-a-unique-ptr
Change Description
Since delete just marks the memory as "being free for reuse" and lhost does not immediately go out of scope and is used in several places it is safer to use std::unique_ptr instead of naked pointers and the corresponding delete
Change Type
Select ONE
Consensus Changes
API Changes
Documentation Additions