search

abedra / libvault

A C++ library for Hashicorp Vault
MIT License
34 stars 25 forks source link

Wrap curl objects in std::unique_ptr #89

Closed Blzut3 closed 2 years ago

Blzut3 commented 3 years ago

This prevents memory leaks if the callbacks throw exceptions (most notably the errorCallback).

Blzut3 commented 2 years ago

@abedra Do you have any comments on this change?

abedra commented 2 years ago

@abedra Do you have any comments on this change?

Apologies for losing track of this one. I understand your point and agree that this could lead to undesired impact. Going to take another look through this and consider a couple different paths as well

abedra commented 2 years ago

I ended up going a little further on this idea and completely encapsulated the underlying curl pieces. The resulting commit is at d47c31eb01d51b7cd142b6c4ef893f604914c497. Thanks for pointing this out, I do think this was a necessary improvement.

Blzut3 commented 2 years ago

Thanks for getting this resolved! Will try it out soon. (Nitpick: you probably could have just used a stack allocated CurlWrapper instead of unique_ptr.)

abedra commented 2 years ago

Yeah, fair point on the stack allocation vs the unique_ptr. Things morphed a bit throughout the lifecycle of the PR and now that it's all settled it does appear safe to move in that direction.