From d087c9605ffffdc9053c5efdc83f53658afbe9a6 Mon Sep 17 00:00:00 2001 From: Colin Snover Date: Fri, 10 Nov 2017 23:06:42 -0600 Subject: BASE: Remove bad casts between incompatible Plugin types Previously, a C-style cast was used to convert a Common::Array, populated with pointers to StaticPlugin and DynamicPlugin instances, to a Common::Array *>, but PluginSubclass is a *sibling* class to StaticPlugin/DynamicPlugin, so this cast was invalid and the results undefined. The methods for retrieving subclasses of plugins can't be easily changed to just generate an array of temporary wrapper objects that expose an identical API which dereferences to the preferred PluginObject subclass because pointers to these objects are retained by other parts of ScummVM, so the wrappers would needed to be persisted or they would need to just re-expose the underlying Plugin object again. This indicated that a way to solve this problem is to have the callers receive Plugin objects and get the PluginObject from the Plugin by explicitly stating their desired type, in a similar manner to std::get(std::variant), so that the pattern used by this patch to solve the problem. Closes gh-1051. --- base/commandLine.cpp | 21 ++++++++++++--------- base/main.cpp | 13 +++++++------ base/plugins.cpp | 24 ++++++++++++------------ base/plugins.h | 28 +++++++++------------------- 4 files changed, 40 insertions(+), 46 deletions(-) (limited to 'base') diff --git a/base/commandLine.cpp b/base/commandLine.cpp index 640daa1793..2e981fb75a 100644 --- a/base/commandLine.cpp +++ b/base/commandLine.cpp @@ -673,9 +673,9 @@ static void listGames() { printf("Game ID Full Title \n" "-------------------- ------------------------------------------------------\n"); - const EnginePlugin::List &plugins = EngineMan.getPlugins(); - for (EnginePlugin::List::const_iterator iter = plugins.begin(); iter != plugins.end(); ++iter) { - GameList list = (**iter)->getSupportedGames(); + const PluginList &plugins = EngineMan.getPlugins(); + for (PluginList::const_iterator iter = plugins.begin(); iter != plugins.end(); ++iter) { + GameList list = (*iter)->get().getSupportedGames(); for (GameList::iterator v = list.begin(); v != list.end(); ++v) { printf("%-20s %s\n", v->gameid().c_str(), v->description().c_str()); } @@ -741,7 +741,7 @@ static Common::Error listSaves(const char *target) { gameid.toLowercase(); // Normalize it to lower case // Find the plugin that will handle the specified gameid - const EnginePlugin *plugin = 0; + const Plugin *plugin = nullptr; GameDescriptor game = EngineMan.findGame(gameid, &plugin); if (!plugin) { @@ -749,13 +749,15 @@ static Common::Error listSaves(const char *target) { Common::String::format("target '%s', gameid '%s", target, gameid.c_str())); } - if (!(*plugin)->hasFeature(MetaEngine::kSupportsListSaves)) { + const MetaEngine &metaEngine = plugin->get(); + + if (!metaEngine.hasFeature(MetaEngine::kSupportsListSaves)) { // TODO: Include more info about the target (desc, engine name, ...) ??? return Common::Error(Common::kEnginePluginNotSupportSaves, Common::String::format("target '%s', gameid '%s", target, gameid.c_str())); } else { // Query the plugin for a list of saved games - SaveStateList saveList = (*plugin)->listSaves(target); + SaveStateList saveList = metaEngine.listSaves(target); if (saveList.size() > 0) { // TODO: Include more info about the target (desc, engine name, ...) ??? @@ -793,13 +795,14 @@ static void listThemes() { /** Lists all output devices */ static void listAudioDevices() { - MusicPlugin::List pluginList = MusicMan.getPlugins(); + PluginList pluginList = MusicMan.getPlugins(); printf("ID Description\n"); printf("------------------------------ ------------------------------------------------\n"); - for (MusicPlugin::List::const_iterator i = pluginList.begin(), iend = pluginList.end(); i != iend; ++i) { - MusicDevices deviceList = (**i)->getDevices(); + for (PluginList::const_iterator i = pluginList.begin(), iend = pluginList.end(); i != iend; ++i) { + const MusicPluginObject &musicObject = (*i)->get(); + MusicDevices deviceList = musicObject.getDevices(); for (MusicDevices::iterator j = deviceList.begin(), jend = deviceList.end(); j != jend; ++j) { printf("%-30s %s\n", Common::String::format("\"%s\"", j->getCompleteId().c_str()).c_str(), j->getCompleteName().c_str()); } diff --git a/base/main.cpp b/base/main.cpp index 4251c2c678..f529f3ec08 100644 --- a/base/main.cpp +++ b/base/main.cpp @@ -106,8 +106,8 @@ static bool launcherDialog() { return (dlg.runModal() != -1); } -static const EnginePlugin *detectPlugin() { - const EnginePlugin *plugin = 0; +static const Plugin *detectPlugin() { + const Plugin *plugin = nullptr; // Make sure the gameid is set in the config manager, and that it is lowercase. Common::String gameid(ConfMan.getActiveDomainName()); @@ -141,7 +141,7 @@ static const EnginePlugin *detectPlugin() { } // TODO: specify the possible return values here -static Common::Error runGame(const EnginePlugin *plugin, OSystem &system, const Common::String &edebuglevels) { +static Common::Error runGame(const Plugin *plugin, OSystem &system, const Common::String &edebuglevels) { // Determine the game data path, for validation and error messages Common::FSNode dir(ConfMan.get("path")); Common::Error err = Common::kNoError; @@ -169,15 +169,16 @@ static Common::Error runGame(const EnginePlugin *plugin, OSystem &system, const // Create the game engine if (err.getCode() == Common::kNoError) { + const MetaEngine &metaEngine = plugin->get(); // Set default values for all of the custom engine options // Appareantly some engines query them in their constructor, thus we // need to set this up before instance creation. - const ExtraGuiOptions engineOptions = (*plugin)->getExtraGuiOptions(Common::String()); + const ExtraGuiOptions engineOptions = metaEngine.getExtraGuiOptions(Common::String()); for (uint i = 0; i < engineOptions.size(); i++) { ConfMan.registerDefault(engineOptions[i].configOption, engineOptions[i].defaultState); } - err = (*plugin)->createInstance(&system, &engine); + err = metaEngine.createInstance(&system, &engine); } // Check for errors @@ -504,7 +505,7 @@ extern "C" int scummvm_main(int argc, const char * const argv[]) { // cleanly, so this is now enabled to encourage people to fix bits :) while (0 != ConfMan.getActiveDomain()) { // Try to find a plugin which feels responsible for the specified game. - const EnginePlugin *plugin = detectPlugin(); + const Plugin *plugin = detectPlugin(); if (plugin) { // Unload all plugins not needed for this game, // to save memory diff --git a/base/plugins.cpp b/base/plugins.cpp index 39aaf2f73e..18755003b4 100644 --- a/base/plugins.cpp +++ b/base/plugins.cpp @@ -457,7 +457,7 @@ DECLARE_SINGLETON(EngineManager); * For the uncached version, we first try to find the plugin using the gameId * and only if we can't find it there, we loop through the plugins. **/ -GameDescriptor EngineManager::findGame(const Common::String &gameName, const EnginePlugin **plugin) const { +GameDescriptor EngineManager::findGame(const Common::String &gameName, const Plugin **plugin) const { GameDescriptor result; // First look for the game using the plugins in memory. This is critical @@ -493,18 +493,18 @@ GameDescriptor EngineManager::findGame(const Common::String &gameName, const Eng /** * Find the game within the plugins loaded in memory **/ -GameDescriptor EngineManager::findGameInLoadedPlugins(const Common::String &gameName, const EnginePlugin **plugin) const { +GameDescriptor EngineManager::findGameInLoadedPlugins(const Common::String &gameName, const Plugin **plugin) const { // Find the GameDescriptor for this target - const EnginePlugin::List &plugins = getPlugins(); + const PluginList &plugins = getPlugins(); GameDescriptor result; if (plugin) *plugin = 0; - EnginePlugin::List::const_iterator iter; + PluginList::const_iterator iter; for (iter = plugins.begin(); iter != plugins.end(); ++iter) { - result = (**iter)->findGame(gameName.c_str()); + result = (*iter)->get().findGame(gameName.c_str()); if (!result.gameid().empty()) { if (plugin) *plugin = *iter; @@ -516,22 +516,22 @@ GameDescriptor EngineManager::findGameInLoadedPlugins(const Common::String &game GameList EngineManager::detectGames(const Common::FSList &fslist) const { GameList candidates; - EnginePlugin::List plugins; - EnginePlugin::List::const_iterator iter; + PluginList plugins; + PluginList::const_iterator iter; PluginManager::instance().loadFirstPlugin(); do { plugins = getPlugins(); // Iterate over all known games and for each check if it might be // the game in the presented directory. for (iter = plugins.begin(); iter != plugins.end(); ++iter) { - candidates.push_back((**iter)->detectGames(fslist)); + candidates.push_back((*iter)->get().detectGames(fslist)); } } while (PluginManager::instance().loadNextPlugin()); return candidates; } -const EnginePlugin::List &EngineManager::getPlugins() const { - return (const EnginePlugin::List &)PluginManager::instance().getPlugins(PLUGIN_TYPE_ENGINE); +const PluginList &EngineManager::getPlugins() const { + return PluginManager::instance().getPlugins(PLUGIN_TYPE_ENGINE); } @@ -543,6 +543,6 @@ namespace Common { DECLARE_SINGLETON(MusicManager); } -const MusicPlugin::List &MusicManager::getPlugins() const { - return (const MusicPlugin::List &)PluginManager::instance().getPlugins(PLUGIN_TYPE_MUSIC); +const PluginList &MusicManager::getPlugins() const { + return PluginManager::instance().getPlugins(PLUGIN_TYPE_MUSIC); } diff --git a/base/plugins.h b/base/plugins.h index 2793ff3ffd..ce45f7102a 100644 --- a/base/plugins.h +++ b/base/plugins.h @@ -190,6 +190,15 @@ public: PluginType getType() const; const char *getName() const; + template + T &get() const { + T *pluginObject = dynamic_cast(_pluginObject); + if (!pluginObject) { + error("Invalid cast of plugin %s", getName()); + } + return *pluginObject; + } + /** * The getFileName() function gets the name of the plugin file for those * plugins that have files (ie. not static). It doesn't require the plugin @@ -201,25 +210,6 @@ public: /** List of Plugin instances. */ typedef Common::Array PluginList; -/** - * Convenience template to make it easier defining normal Plugin - * subclasses. Namely, the PluginSubclass will manage PluginObjects - * of a type specified via the PO_t template parameter. - */ -template -class PluginSubclass : public Plugin { -public: - PO_t &operator*() const { - return *(PO_t *)_pluginObject; - } - - PO_t *operator->() const { - return (PO_t *)_pluginObject; - } - - typedef Common::Array List; -}; - /** * Abstract base class for Plugin factories. Subclasses of this * are responsible for creating plugin objects, e.g. by loading -- cgit v1.2.3