Modernize memory management of Core Foundation pointers #3164
Conversation
ChrisThrasher
force-pushed
the
cf_pointers
branch
2 times, most recently
from
8018ff4 to
2abfc25
Compare
Pull Request Test Coverage Report for Build 12539613077Details
💛 - Coveralls |
| @@ -40,7 +41,7 @@ | |||
| namespace sf::priv | |||
| { | |||
|
|
|||
| using IOHIDElements = std::vector<IOHIDElementRef>; | |||
| using IOHIDElements = std::vector<CFPtr<__IOHIDElement>>; | |||
There was a problem hiding this comment.
I don't know much about Obj-C, but should we really be using types that start with __?
There was a problem hiding this comment.
I was hesitant to use types with those names too. Does Apple not want users to directly use such types? I'm not sure. A much more verbose alternative would be std::remove_pointer_t<IOHIDElementRef>.
There was a problem hiding this comment.
I'd probably go with that. Not a fan of using "reserved"/internal symbols
ChrisThrasher
force-pushed
the
cf_pointers
branch
from
2abfc25 to
2853468
Compare
|
Moving this to the v3.1 milestone. I want to finish this eventually but for now it's not a priority to land in v3.0. |
ChrisThrasher
force-pushed
the
cf_pointers
branch
2 times, most recently
from
568ba5f to
e432237
Compare
ChrisThrasher
force-pushed
the
cf_pointers
branch
2 times, most recently
from
d29cdfa to
7f43eb8
Compare
ChrisThrasher
force-pushed
the
cf_pointers
branch
from
7f43eb8 to
d632a09
Compare
ChrisThrasher
force-pushed
the
cf_pointers
branch
from
d632a09 to
fc7a78c
Compare
|
Just curious but could this be wrapped in a simple std::shared_ptr type class? Hell even unique_ptr can be given a function (or in C++17 case a class) that could call release once its done with the pointer. |
Description
Follows the same pattern as #2540, #2759, #2801, and #3149.
Core Foundation pointers are used in a slightly more complicated way than in the previously cited PRs. In some cases
CFRetainis called on a given pointer. I don't entirely understand how retains and releases work in the Apple Objective-C world so I chose not to touch pointers which are retained.I believe this fixes a memory leak here:
SFML/src/SFML/Window/macOS/HIDInputManager.mm
Lines 771 to 776 in 5eb57ae
If
underlyingis non-null andCFArrayGetCount(underlying)returns zero then we exit the function without releasingunderlying. This PR's new RAII-based approach guarantees thatunderlyinggets freed.Edit: I tried improving upon the pointers that did use
CFRetainnow that I better understand what retains and releases are. I'm still not entirely what I wrote is correct so this PR will have to sit for a bit longer before I'm ready to mark it as ready for review.