From def8d7645b26d491fb1b78617e23c72b25630814 Mon Sep 17 00:00:00 2001 From: Chris Thrasher Date: Sat, 6 Jul 2024 13:26:41 -0600 Subject: [PATCH] Modernize memory management of Core Foundation pointers --- src/SFML/Window/macOS/HIDInputManager.hpp | 9 +-- src/SFML/Window/macOS/HIDInputManager.mm | 61 +++++++------------- src/SFML/Window/macOS/HIDJoystickManager.cpp | 21 ++++--- src/SFML/Window/macOS/HIDJoystickManager.hpp | 4 +- src/SFML/Window/macOS/InputImpl.mm | 12 ++-- src/SFML/Window/macOS/JoystickImpl.cpp | 38 ++++-------- src/SFML/Window/macOS/Utils.hpp | 54 +++++++++++++++++ src/SFML/Window/macOS/VideoModeImpl.cpp | 10 ++-- src/SFML/Window/macOS/cg_sf_conversion.mm | 22 +++---- 9 files changed, 124 insertions(+), 107 deletions(-) create mode 100644 src/SFML/Window/macOS/Utils.hpp diff --git a/src/SFML/Window/macOS/HIDInputManager.hpp b/src/SFML/Window/macOS/HIDInputManager.hpp index 2b078fd0c..d7cd1f2c2 100644 --- a/src/SFML/Window/macOS/HIDInputManager.hpp +++ b/src/SFML/Window/macOS/HIDInputManager.hpp @@ -30,6 +30,7 @@ //////////////////////////////////////////////////////////// #include #include +#include #include #include @@ -243,10 +244,10 @@ private: /// /// \param page HID page like kHIDPage_GenericDesktop /// \param usage HID usage page like kHIDUsage_GD_Keyboard or kHIDUsage_GD_Mouse - /// \return a retained, non-empty CFSetRef of IOHIDDeviceRef or a null pointer + /// \return a retained, non-empty __CFSet pointer of IOHIDDeviceRef or a null pointer /// //////////////////////////////////////////////////////////// - CFSetRef copyDevices(std::uint32_t page, std::uint32_t usage); + CFPtr copyDevices(std::uint32_t page, std::uint32_t usage); //////////////////////////////////////////////////////////// /// \brief Check if a key is pressed @@ -286,8 +287,8 @@ private: //////////////////////////////////////////////////////////// // Member data //////////////////////////////////////////////////////////// - IOHIDManagerRef m_manager{}; ///< Underlying HID Manager - bool m_keysInitialized{}; ///< Has initializeKeyboard been called at least once? + CFPtr<__IOHIDManager> m_manager{}; ///< Underlying HID Manager + bool m_keysInitialized{}; ///< Has initializeKeyboard been called at least once? EnumArray m_keys; ///< All the keys on any connected keyboard EnumArray m_keyToScancodeMapping{}; ///< Mapping from Key to Scancode EnumArray m_scancodeToKeyMapping{}; ///< Mapping from Scancode to Key diff --git a/src/SFML/Window/macOS/HIDInputManager.mm b/src/SFML/Window/macOS/HIDInputManager.mm index 829901f55..18d04a129 100644 --- a/src/SFML/Window/macOS/HIDInputManager.mm +++ b/src/SFML/Window/macOS/HIDInputManager.mm @@ -78,14 +78,12 @@ CFDictionaryRef HIDInputManager::copyDevicesMask(std::uint32_t page, std::uint32 &kCFTypeDictionaryValueCallBacks); // Add the page value. - CFNumberRef value = CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &page); - CFDictionarySetValue(dict, CFSTR(kIOHIDDeviceUsagePageKey), value); - CFRelease(value); + auto value = CFPtr(CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &page)); + CFDictionarySetValue(dict, CFSTR(kIOHIDDeviceUsagePageKey), value.get()); // Add the usage value (which is only valid if page value exists). - value = CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &usage); - CFDictionarySetValue(dict, CFSTR(kIOHIDDeviceUsageKey), value); - CFRelease(value); + value = CFPtr(CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &usage)); + CFDictionarySetValue(dict, CFSTR(kIOHIDDeviceUsageKey), value.get()); return dict; } @@ -712,8 +710,8 @@ String HIDInputManager::getDescription(Keyboard::Scancode code) HIDInputManager::HIDInputManager() { // Create an HID Manager reference - m_manager = IOHIDManagerCreate(kCFAllocatorDefault, kIOHIDOptionsTypeNone); - const IOReturn openStatus = IOHIDManagerOpen(m_manager, kIOHIDOptionsTypeNone); + m_manager = CFPtr<__IOHIDManager>(IOHIDManagerCreate(kCFAllocatorDefault, kIOHIDOptionsTypeNone)); + const IOReturn openStatus = IOHIDManagerOpen(m_manager.get(), kIOHIDOptionsTypeNone); if (openStatus != kIOReturnSuccess) { @@ -752,7 +750,7 @@ void HIDInputManager::initializeKeyboard() // in approximately constant time. // Get only keyboards - CFSetRef underlying = copyDevices(kHIDPage_GenericDesktop, kHIDUsage_GD_Keyboard); + const auto underlying = copyDevices(kHIDPage_GenericDesktop, kHIDUsage_GD_Keyboard); if (underlying == nullptr) { err() << "No keyboard detected by the HID manager!" << std::endl; @@ -760,12 +758,10 @@ void HIDInputManager::initializeKeyboard() return; } - auto* const keyboards = static_cast(underlying); // Toll-Free Bridge - for (id keyboard in keyboards) // NOLINT(cppcoreguidelines-init-variables) + auto* const keyboards = static_cast(underlying.get()); // Toll-Free Bridge + for (id keyboard in keyboards) // NOLINT(cppcoreguidelines-init-variables) loadKeyboard(static_cast(keyboard)); - CFRelease(underlying); - //////////////////////////////////////////////////////////// // At this point m_keys is filled with as many IOHIDElementRef as possible } @@ -774,22 +770,21 @@ void HIDInputManager::initializeKeyboard() //////////////////////////////////////////////////////////// void HIDInputManager::loadKeyboard(IOHIDDeviceRef keyboard) { - CFArrayRef underlying = IOHIDDeviceCopyMatchingElements(keyboard, nullptr, kIOHIDOptionsTypeNone); - if ((underlying == nullptr) || (CFArrayGetCount(underlying) == 0)) + const auto underlying = CFPtr( + IOHIDDeviceCopyMatchingElements(keyboard, nullptr, kIOHIDOptionsTypeNone)); + if ((underlying == nullptr) || (CFArrayGetCount(underlying.get()) == 0)) { err() << "Detected a keyboard without any keys." << std::endl; return; } - auto* const keys = static_cast(underlying); // Toll-Free Bridge - for (id key in keys) // NOLINT(cppcoreguidelines-init-variables) + auto* const keys = static_cast(underlying.get()); // Toll-Free Bridge + for (id key in keys) // NOLINT(cppcoreguidelines-init-variables) { auto* elem = static_cast(key); if (IOHIDElementGetUsagePage(elem) == kHIDPage_KeyboardOrKeypad) loadKey(elem); } - - CFRelease(underlying); } @@ -814,13 +809,12 @@ void HIDInputManager::buildMappings() m_scancodeToKeyMapping.fill(Keyboard::Key::Unknown); // Get the current keyboard layout - TISInputSourceRef tis = TISCopyCurrentKeyboardLayoutInputSource(); - const auto* layoutData = static_cast(TISGetInputSourceProperty(tis, kTISPropertyUnicodeKeyLayoutData)); + const auto tis = CFPtr<__TISInputSource>(TISCopyCurrentKeyboardLayoutInputSource()); + const auto* layoutData = static_cast(TISGetInputSourceProperty(tis.get(), kTISPropertyUnicodeKeyLayoutData)); if (layoutData == nullptr) { err() << "Cannot get the keyboard layout\n"; - CFRelease(tis); return; } @@ -905,8 +899,6 @@ void HIDInputManager::buildMappings() m_keyToScancodeMapping[code] = scan; m_scancodeToKeyMapping[scan] = code; } - - CFRelease(tis); } @@ -925,10 +917,7 @@ void HIDInputManager::keyboardChanged(CFNotificationCenterRef /* center */, //////////////////////////////////////////////////////////// void HIDInputManager::freeUp() { - if (m_manager != nil) - CFRelease(m_manager); - - m_manager = nil; + m_manager.reset(); if (m_keysInitialized) { @@ -945,26 +934,20 @@ void HIDInputManager::freeUp() //////////////////////////////////////////////////////////// -CFSetRef HIDInputManager::copyDevices(std::uint32_t page, std::uint32_t usage) +CFPtr HIDInputManager::copyDevices(std::uint32_t page, std::uint32_t usage) { // Filter and keep only the requested devices - CFDictionaryRef mask = copyDevicesMask(page, usage); + const auto mask = CFPtr(copyDevicesMask(page, usage)); - IOHIDManagerSetDeviceMatching(m_manager, mask); + IOHIDManagerSetDeviceMatching(m_manager.get(), mask.get()); - CFRelease(mask); - mask = nil; - - CFSetRef devices = IOHIDManagerCopyDevices(m_manager); + auto devices = CFPtr(IOHIDManagerCopyDevices(m_manager.get())); if (devices == nullptr) return nullptr; // Is there at least one device? - if (CFSetGetCount(devices) < 1) - { - CFRelease(devices); + if (CFSetGetCount(devices.get()) < 1) return nullptr; - } return devices; } diff --git a/src/SFML/Window/macOS/HIDJoystickManager.cpp b/src/SFML/Window/macOS/HIDJoystickManager.cpp index 3c05db8cc..b203c0c58 100644 --- a/src/SFML/Window/macOS/HIDJoystickManager.cpp +++ b/src/SFML/Window/macOS/HIDJoystickManager.cpp @@ -61,10 +61,9 @@ unsigned int HIDJoystickManager::getJoystickCount() //////////////////////////////////////////////////////////// -CFSetRef HIDJoystickManager::copyJoysticks() +CFPtr HIDJoystickManager::copyJoysticks() { - CFSetRef devices = IOHIDManagerCopyDevices(m_manager); - return devices; + return CFPtr(IOHIDManagerCopyDevices(m_manager)); } @@ -73,17 +72,17 @@ HIDJoystickManager::HIDJoystickManager() { m_manager = IOHIDManagerCreate(kCFAllocatorDefault, kIOHIDOptionsTypeNone); - CFDictionaryRef mask0 = HIDInputManager::copyDevicesMask(kHIDPage_GenericDesktop, kHIDUsage_GD_Joystick); + const auto mask0 = CFPtr( + HIDInputManager::copyDevicesMask(kHIDPage_GenericDesktop, kHIDUsage_GD_Joystick)); - CFDictionaryRef mask1 = HIDInputManager::copyDevicesMask(kHIDPage_GenericDesktop, kHIDUsage_GD_GamePad); + const auto mask1 = CFPtr( + HIDInputManager::copyDevicesMask(kHIDPage_GenericDesktop, kHIDUsage_GD_GamePad)); - std::array maskArray = {mask0, mask1}; - CFArrayRef mask = CFArrayCreate(nullptr, reinterpret_cast(maskArray.data()), maskArray.size(), nullptr); + std::array maskArray = {mask0.get(), mask1.get()}; + const auto mask = CFPtr( + CFArrayCreate(nullptr, reinterpret_cast(maskArray.data()), maskArray.size(), nullptr)); - IOHIDManagerSetDeviceMatchingMultiple(m_manager, mask); - CFRelease(mask); - CFRelease(mask1); - CFRelease(mask0); + IOHIDManagerSetDeviceMatchingMultiple(m_manager, mask.get()); IOHIDManagerRegisterDeviceMatchingCallback(m_manager, pluggedIn, this); diff --git a/src/SFML/Window/macOS/HIDJoystickManager.hpp b/src/SFML/Window/macOS/HIDJoystickManager.hpp index e4f442fa9..620c84639 100644 --- a/src/SFML/Window/macOS/HIDJoystickManager.hpp +++ b/src/SFML/Window/macOS/HIDJoystickManager.hpp @@ -75,10 +75,10 @@ public: //////////////////////////////////////////////////////////// /// \brief Copy the devices associated with this HID manager /// - /// \return a retained CFSetRef of IOHIDDeviceRef or a null pointer + /// \return a retained __CFSet pointer of IOHIDDeviceRef or a null pointer /// //////////////////////////////////////////////////////////// - CFSetRef copyJoysticks(); + CFPtr copyJoysticks(); private: //////////////////////////////////////////////////////////// diff --git a/src/SFML/Window/macOS/InputImpl.mm b/src/SFML/Window/macOS/InputImpl.mm index 1ab739673..5944f3870 100644 --- a/src/SFML/Window/macOS/InputImpl.mm +++ b/src/SFML/Window/macOS/InputImpl.mm @@ -212,12 +212,12 @@ void setMousePosition(Vector2i position) const CGPoint pos = CGPointMake(position.x / scale, position.y / scale); // Place the cursor. - CGEventRef event = CGEventCreateMouseEvent(nullptr, - kCGEventMouseMoved, - pos, - /* we don't care about this: */ kCGMouseButtonLeft); - CGEventPost(kCGHIDEventTap, event); - CFRelease(event); + const auto event = CFPtr<__CGEvent>( + CGEventCreateMouseEvent(nullptr, + kCGEventMouseMoved, + pos, + /* we don't care about this: */ kCGMouseButtonLeft)); + CGEventPost(kCGHIDEventTap, event.get()); // This is a workaround to deprecated CGSetLocalEventsSuppressionInterval. } diff --git a/src/SFML/Window/macOS/JoystickImpl.cpp b/src/SFML/Window/macOS/JoystickImpl.cpp index e380306d4..de62d5e69 100644 --- a/src/SFML/Window/macOS/JoystickImpl.cpp +++ b/src/SFML/Window/macOS/JoystickImpl.cpp @@ -132,15 +132,15 @@ bool JoystickImpl::isConnected(unsigned int index) if (connectedCount > openedCount) { // Get all devices - CFSetRef devices = HIDJoystickManager::getInstance().copyJoysticks(); + const auto devices = CFPtr(HIDJoystickManager::getInstance().copyJoysticks()); if (devices != nullptr) { - const CFIndex size = CFSetGetCount(devices); + const CFIndex size = CFSetGetCount(devices.get()); if (size > 0) { std::vector array(static_cast(size)); // array of IOHIDDeviceRef - CFSetGetValues(devices, array.data()); + CFSetGetValues(devices.get(), array.data()); // If there exists a device d s.t. there is no j s.t. // m_locationIDs[j] == d's location then we have a new device. @@ -166,8 +166,6 @@ bool JoystickImpl::isConnected(unsigned int index) } } } - - CFRelease(devices); } } } @@ -185,14 +183,14 @@ bool JoystickImpl::open(unsigned int index) const Location deviceLoc = m_locationIDs[index]; // The device we need to load // Get all devices - CFSetRef devices = HIDJoystickManager::getInstance().copyJoysticks(); + const auto devices = HIDJoystickManager::getInstance().copyJoysticks(); if (devices == nullptr) return false; // Get a usable copy of the joysticks devices. - const CFIndex joysticksCount = CFSetGetCount(devices); + const CFIndex joysticksCount = CFSetGetCount(devices.get()); std::vector devicesArray(static_cast(joysticksCount)); - CFSetGetValues(devices, devicesArray.data()); + CFSetGetValues(devices.get(), devicesArray.data()); // Get the desired joystick. IOHIDDeviceRef self = nil; @@ -204,29 +202,23 @@ bool JoystickImpl::open(unsigned int index) } if (self == nil) - { - CFRelease(devices); return false; - } m_identification.name = getDeviceString(self, CFSTR(kIOHIDProductKey), m_index); m_identification.vendorId = getDeviceUint(self, CFSTR(kIOHIDVendorIDKey), m_index); m_identification.productId = getDeviceUint(self, CFSTR(kIOHIDProductIDKey), m_index); // Get a list of all elements attached to the device. - CFArrayRef elements = IOHIDDeviceCopyMatchingElements(self, nullptr, kIOHIDOptionsTypeNone); + const auto elements = CFPtr(IOHIDDeviceCopyMatchingElements(self, nullptr, kIOHIDOptionsTypeNone)); if (elements == nullptr) - { - CFRelease(devices); return false; - } // Go through all connected elements. - const CFIndex elementsCount = CFArrayGetCount(elements); + const CFIndex elementsCount = CFArrayGetCount(elements.get()); for (int i = 0; i < elementsCount; ++i) { - auto* element = static_cast(const_cast(CFArrayGetValueAtIndex(elements, i))); + auto* element = static_cast(const_cast(CFArrayGetValueAtIndex(elements.get(), i))); switch (IOHIDElementGetUsagePage(element)) { case kHIDPage_GenericDesktop: @@ -331,9 +323,6 @@ bool JoystickImpl::open(unsigned int index) // Axis X (for example) and we want to keep only the last one. To prevent // leaking we retain objects 'only' now. - CFRelease(devices); - CFRelease(elements); - return true; } @@ -406,14 +395,14 @@ JoystickState JoystickImpl::update() const Location selfLoc = m_locationIDs[m_index]; // Get all devices - CFSetRef devices = HIDJoystickManager::getInstance().copyJoysticks(); + auto devices = HIDJoystickManager::getInstance().copyJoysticks(); if (devices == nullptr) return disconnectedState; // Get a usable copy of the joysticks devices. - const CFIndex joysticksCount = CFSetGetCount(devices); + const CFIndex joysticksCount = CFSetGetCount(devices.get()); std::vector devicesArray(static_cast(joysticksCount)); - CFSetGetValues(devices, devicesArray.data()); + CFSetGetValues(devices.get(), devicesArray.data()); // Search for it bool found = false; @@ -424,9 +413,6 @@ JoystickState JoystickImpl::update() found = true; } - // Release unused stuff - CFRelease(devices); - // If not found we consider it disconnected if (!found) return disconnectedState; diff --git a/src/SFML/Window/macOS/Utils.hpp b/src/SFML/Window/macOS/Utils.hpp new file mode 100644 index 000000000..a877bb14a --- /dev/null +++ b/src/SFML/Window/macOS/Utils.hpp @@ -0,0 +1,54 @@ +//////////////////////////////////////////////////////////// +// +// SFML - Simple and Fast Multimedia Library +// Copyright (C) 2007-2024 Laurent Gomila (laurent@sfml-dev.org) +// +// This software is provided 'as-is', without any express or implied warranty. +// In no event will the authors be held liable for any damages arising from the use of this software. +// +// Permission is granted to anyone to use this software for any purpose, +// including commercial applications, and to alter it and redistribute it freely, +// subject to the following restrictions: +// +// 1. The origin of this software must not be misrepresented; +// you must not claim that you wrote the original software. +// If you use this software in a product, an acknowledgment +// in the product documentation would be appreciated but is not required. +// +// 2. Altered source versions must be plainly marked as such, +// and must not be misrepresented as being the original software. +// +// 3. This notice may not be removed or altered from any source distribution. +// +//////////////////////////////////////////////////////////// + +#pragma once + +//////////////////////////////////////////////////////////// +// Headers +//////////////////////////////////////////////////////////// +#include + + +namespace sf::priv +{ +//////////////////////////////////////////////////////////// +/// \brief Class for freeing Core Foundation pointers +/// +//////////////////////////////////////////////////////////// +struct CFDeleter +{ + template + void operator()(T* data) const + { + CFRelease(data); + } +}; + +//////////////////////////////////////////////////////////// +/// \brief Class template for wrapping owning raw pointers from Core Foundation +/// +//////////////////////////////////////////////////////////// +template +using CFPtr = std::unique_ptr; +} // namespace sf::priv diff --git a/src/SFML/Window/macOS/VideoModeImpl.cpp b/src/SFML/Window/macOS/VideoModeImpl.cpp index 371558dee..40203e8ab 100644 --- a/src/SFML/Window/macOS/VideoModeImpl.cpp +++ b/src/SFML/Window/macOS/VideoModeImpl.cpp @@ -27,6 +27,7 @@ // Headers //////////////////////////////////////////////////////////// #include +#include #include #include @@ -43,7 +44,7 @@ std::vector VideoModeImpl::getFullscreenModes() std::vector modes; // Retrieve all modes available for main screen only. - CFArrayRef cgmodes = CGDisplayCopyAllDisplayModes(CGMainDisplayID(), nullptr); + const auto cgmodes = CFPtr(CGDisplayCopyAllDisplayModes(CGMainDisplayID(), nullptr)); if (cgmodes == nullptr) { @@ -54,10 +55,10 @@ std::vector VideoModeImpl::getFullscreenModes() const VideoMode desktop = getDesktopMode(); // Loop on each mode and convert it into a sf::VideoMode object. - const CFIndex modesCount = CFArrayGetCount(cgmodes); + const CFIndex modesCount = CFArrayGetCount(cgmodes.get()); for (CFIndex i = 0; i < modesCount; ++i) { - auto* cgmode = static_cast(const_cast(CFArrayGetValueAtIndex(cgmodes, i))); + auto* cgmode = static_cast(const_cast(CFArrayGetValueAtIndex(cgmodes.get(), i))); const VideoMode mode = convertCGModeToSFMode(cgmode); @@ -70,9 +71,6 @@ std::vector VideoModeImpl::getFullscreenModes() modes.push_back(mode); } - // Clean up memory. - CFRelease(cgmodes); - return modes; } diff --git a/src/SFML/Window/macOS/cg_sf_conversion.mm b/src/SFML/Window/macOS/cg_sf_conversion.mm index 7c9f39d37..ac5f363f1 100644 --- a/src/SFML/Window/macOS/cg_sf_conversion.mm +++ b/src/SFML/Window/macOS/cg_sf_conversion.mm @@ -27,6 +27,7 @@ // Headers //////////////////////////////////////////////////////////// #import +#include #include #pragma GCC diagnostic ignored "-Wdeprecated-declarations" @@ -37,21 +38,16 @@ namespace sf::priv //////////////////////////////////////////////////////////// unsigned int modeBitsPerPixel(CGDisplayModeRef mode) { - unsigned int bpp = 0; // no match - // Compare encoding. - CFStringRef pixEnc = CGDisplayModeCopyPixelEncoding(mode); - if (CFStringCompare(pixEnc, CFSTR(IO32BitDirectPixels), kCFCompareCaseInsensitive) == kCFCompareEqualTo) - bpp = 32; - else if (CFStringCompare(pixEnc, CFSTR(IO16BitDirectPixels), kCFCompareCaseInsensitive) == kCFCompareEqualTo) - bpp = 16; - else if (CFStringCompare(pixEnc, CFSTR(IO8BitIndexedPixels), kCFCompareCaseInsensitive) == kCFCompareEqualTo) - bpp = 8; + const auto pixEnc = CFPtr(CGDisplayModeCopyPixelEncoding(mode)); + if (CFStringCompare(pixEnc.get(), CFSTR(IO32BitDirectPixels), kCFCompareCaseInsensitive) == kCFCompareEqualTo) + return 32; + if (CFStringCompare(pixEnc.get(), CFSTR(IO16BitDirectPixels), kCFCompareCaseInsensitive) == kCFCompareEqualTo) + return 16; + if (CFStringCompare(pixEnc.get(), CFSTR(IO8BitIndexedPixels), kCFCompareCaseInsensitive) == kCFCompareEqualTo) + return 8; - // Clean up memory. - CFRelease(pixEnc); - - return bpp; + return 0; }