Fix Windows IMMNotificationClient having non-virtual destructor warning#30
Merged
DanielaOrtner merged 1 commit intoJan 27, 2024
Conversation
DanielaOrtner
approved these changes
Jan 27, 2024
Contributor
DanielaOrtner
left a comment
There was a problem hiding this comment.
Thank you for the good work!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
IMMNotificationClientis a Microsoft COM Interface. It is a pure virtual class that does not have a virtual destructor.Windows Audio Session API (WASAPI) uses
IMMNotificationClientto provide notifications when an audio endpoint device is added or removed, when the state or properties of an endpoint device change, or when there is a change in the default role assigned to an endpoint device.[1]To receive notifications when the default audio endpoint device for a particular device role has changed, Rebel Engine passes a reference to an instance of
CMMNotificationClient(an implementation ofIMMNotificationClient) when registering the endpoint notification callback. However,CMMNotificationClientis not implemented as a COM Object. It simply extends the C++ virtual class. This results in theCMMNotificationClientdeclaration warning:This warning alerts against the possible undefined behaviour when deleting a pointer to the base class
IMMNotificationClient, which would not call the destructor ofCMMNotificationClient. This issue is avoided in COM programming by only creating COM Objects usingCoCreateInstanceand making the COM Object responsible for reference counting and deleting itself.[2][3]To ignore the warning in Rebel Engine, we need to independently ensure that
CMMNotificationClientis not deleted via a pointer toIMMNotificationClient. In Rebel Engine,CMMNotificationClientis a static variable; so it cannot be deleted. It is passed by reference (i.e. not a pointer) toRegisterEndpointNotificationCallback().The standard implementation of
IUnknownCOM Interface that all COM Objects inherit, uses theRelease()method to decrement the reference counter and delete itself when the reference counter reaches zero. SinceCMMNotificationClientis not implemented as a COM Object, this PR also ensures thatCMMNotificationClientis never returned as an interface, provides dummy implementations of the otherIUnknownInterface methods, and removes the unused reference counter variable.Finally, it also removes the unused
_pEnumeratorvariable.Fixes godotengine/godot#35194
[1] https://learn.microsoft.com/en-us/windows/win32/api/mmdeviceapi/nn-mmdeviceapi-immnotificationclient
[2] https://learn.microsoft.com/en-us/windows/win32/learnwin32/creating-an-object-in-com
[3] https://learn.microsoft.com/en-us/windows/win32/com/managing-object-lifetimes-through-reference-counting