Skip to content

Core (lv-tool): Protect new display driver instances using unique_ptr #271

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Core (lv-tool): Protect new display driver instances using unique_ptr #271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions libvisual/tools/lv-tool/display/display.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "display.hpp"
#include "display_driver_factory.hpp"
#include <libvisual/libvisual.h>
#include <utility>
#include <stdexcept>
#include <string>

Expand All @@ -34,19 +35,17 @@ class Display::Impl

std::unique_ptr<DisplayDriver> driver;

Impl ()
: driver (nullptr)
Impl (std::unique_ptr<DisplayDriver> driver_)
: driver {std::move (driver_)}
{}

~Impl ()
{}
};

Display::Display (std::string const& driver_name)
: m_impl (new Impl)
: m_impl {new Impl {DisplayDriverFactory::instance().make (driver_name, *this)}}
Copy link
Member

@hartwork hartwork Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

{
m_impl->driver.reset (DisplayDriverFactory::instance().make (driver_name, *this));

if (!m_impl->driver) {
throw std::runtime_error ("Failed to load display driver '" + driver_name + "'. Valid driver set? (\"--driver\" parameter)");
}
Expand Down
4 changes: 2 additions & 2 deletions libvisual/tools/lv-tool/display/display_driver_factory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,15 @@ void DisplayDriverFactory::add_driver (std::string const& name, Creator const& c
m_impl->creators[name] = creator;
}

DisplayDriver* DisplayDriverFactory::make (std::string const& name, Display& display)
std::unique_ptr<DisplayDriver> DisplayDriverFactory::make (std::string const& name, Display& display)
{
auto entry = m_impl->creators.find (name);

if (entry == m_impl->creators.end ()) {
return nullptr;
}

return entry->second (display);
return std::unique_ptr<DisplayDriver> {entry->second (display)};
}

bool DisplayDriverFactory::has_driver (std::string const& name) const
Expand Down
2 changes: 1 addition & 1 deletion libvisual/tools/lv-tool/display/display_driver_factory.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class DisplayDriverFactory
return m_instance;
}

DisplayDriver* make (std::string const& name, Display& display);
std::unique_ptr<DisplayDriver> make (std::string const& name, Display& display);

void add_driver (std::string const& name, Creator const& creator);

Expand Down