Core (lv-tool): Protect new display driver instances using unique_ptr #271
Conversation
This file contains 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
…s with std::unique_ptr<>.
hartwork
reviewed
Feb 10, 2023
| {} | ||
|
|
||
| ~Impl () | ||
| {} | ||
| }; | ||
|
|
||
| Display::Display (std::string const& driver_name) | ||
| : m_impl (new Impl) | ||
| : m_impl {new Impl {DisplayDriverFactory::instance().make (driver_name, *this)}} |
There was a problem hiding this comment.
@kaixiong we are passing *this here while this is in the middle of construction and not ready for any use beyond maybe holding a reference or pointer to it. We are sort-of doing that already before but… do we even need that? If it's out of scope, can it be resolved later?
There was a problem hiding this comment.
@hartwork good catch! I forgot about this but it's definitely safe with the current code since every driver class only stores the reference during their construction. But let me think about it more.
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.
DisplayDriverFactory::make()currently returns driver instances in raw pointers. This patch changes it to wrap them instd::unique_ptr.Note that the various constructors
stdout_driver_new(),stdout_sdl_driver_new(), etc. still return raw pointers. The driver code are meant to be externalised into loadable SOs and I'm unsure if we can returnstd::unique_ptrfrom there.