[WIP] Modernization/migrate to smart pointers #49
Conversation
sierrafoxtrot
left a comment
sierrafoxtrot
left a comment
There was a problem hiding this comment.
Thanks Sean. Definitely keen to move adler to smart pointers but I had some thoughts on the overall approach. Some comments made which describe those.
Happy to tackle when you've had a chance to rebase/merge of course.
| */ | ||
| ~memory_walker_adler16() override = default; | ||
|
|
||
| private: |
There was a problem hiding this comment.
This inclusion of private here is intentional. The idea is that the headers are written from a template and in a very structured way. Rather than mysteriously move the constructor to the bottom of the header, it's left in the usual spot as a prompt (to the lib user especially) that the create method should be used.
| */ | ||
| ~memory_walker_adler32() override = default; | ||
|
|
||
| private: |
There was a problem hiding this comment.
This is intentional, same as in adler16.h above.
| // | ||
| memory_walker_adler32::pointer w = | ||
| memory_walker_adler32::create(); | ||
| auto w = std::make_shared<memory_walker_adler32>(); |
There was a problem hiding this comment.
Definitely keen to move to smart pointers. I should have picked these up when I moved from boost to std share pointers elsewhere.
However, I'd rather move the creation of the smart pointer into the create method in the class(es). My worry is that someone using libsrecord won't get the benefit or more importantly may mix smart and non-smart pointers if they are using base and library code from the project.
|
Hi Sean. Apologies for falling behind but did you still want to move forward with this one? |
|
I have been a little busy with life events recently. Will be coming back to this in a couple weeks. |
|
Completely understand. Nothing but empathy from me on that one. I hope all is well. |
2 participants
Blocked by PR #48
Start migrating code when applicable to:
Goal is to reduce call of new/delete within source code.
To help validate a branch utilizing Valgrind was created. Cherry pick commit as needed for testing.