From 292bfe3f287c6c5b6d7f0df3c5b9edc97e5520c8 Mon Sep 17 00:00:00 2001 From: Alejandro Hernandez Cordero Date: Fri, 17 Jul 2026 11:11:45 +0200 Subject: [PATCH] std::format, concepts, range-for using and string_view Signed-off-by: Alejandro Hernandez Cordero --- pluginlib/include/pluginlib/class_loader.hpp | 22 +++-- .../include/pluginlib/class_loader_imp.hpp | 83 +++++++++---------- 2 files changed, 52 insertions(+), 53 deletions(-) diff --git a/pluginlib/include/pluginlib/class_loader.hpp b/pluginlib/include/pluginlib/class_loader.hpp index 3b564dc..2a28cb4 100644 --- a/pluginlib/include/pluginlib/class_loader.hpp +++ b/pluginlib/include/pluginlib/class_loader.hpp @@ -47,12 +47,20 @@ namespace pluginlib template using UniquePtr = class_loader::ClassLoader::UniquePtr; +/// Satisfied when interface T can be constructed from Args according to its InterfaceTraits. +/** + * Replaces the std::enable_if_t> SFINAE guard + * on the create*Instance() methods with a named constraint, yielding clearer diagnostics. + */ +template +concept InterfaceConstructible = class_loader::is_interface_constructible_v; + /// A class to help manage and load classes. template class ClassLoader : public ClassLoaderBase { public: - typedef typename std::map::iterator ClassMapIterator; + using ClassMapIterator = typename std::map::iterator; /** * \param package The package containing the base class @@ -84,8 +92,8 @@ class ClassLoader : public ClassLoaderBase * \throws pluginlib::CreateClassException when the class cannot be instantiated * \return An instance of the class */ - template, bool> = true> + template + requires InterfaceConstructible std::shared_ptr createSharedInstance(const std::string & lookup_name, Args && ... args); /// Create an instance of a desired class. @@ -106,8 +114,8 @@ class ClassLoader : public ClassLoaderBase * \throws pluginlib::CreateClassException when the class cannot be instantiated * \return An instance of the class */ - template, bool> = true> + template + requires InterfaceConstructible UniquePtr createUniqueInstance(const std::string & lookup_name, Args && ... args); /// Create an instance of a desired class. @@ -126,8 +134,8 @@ class ClassLoader : public ClassLoaderBase * \throws pluginlib::CreateClassException when the class cannot be instantiated * \return An instance of the class */ - template, bool> = true> + template + requires InterfaceConstructible T * createUnmanagedInstance(const std::string & lookup_name, Args && ... args); /// Return a list of all available plugin manifest paths for this ClassLoader's base class type. diff --git a/pluginlib/include/pluginlib/class_loader_imp.hpp b/pluginlib/include/pluginlib/class_loader_imp.hpp index 52c579e..6e80731 100644 --- a/pluginlib/include/pluginlib/class_loader_imp.hpp +++ b/pluginlib/include/pluginlib/class_loader_imp.hpp @@ -35,9 +35,11 @@ #include #include #include +#include // NOLINT(build/include_order) cpplint misclassifies as a C header #include #include #include +#include #include #include @@ -100,8 +102,8 @@ ClassLoader::~ClassLoader() } template -template, bool>> +template +requires InterfaceConstructible std::shared_ptr ClassLoader::createSharedInstance( const std::string & lookup_name, Args &&... args) @@ -111,8 +113,8 @@ std::shared_ptr ClassLoader::createSharedInstance( } template -template, bool>> +template +requires InterfaceConstructible UniquePtr ClassLoader::createUniqueInstance(const std::string & lookup_name, Args &&... args) { RCUTILS_LOG_DEBUG_NAMED("pluginlib.ClassLoader", @@ -146,8 +148,8 @@ UniquePtr ClassLoader::createUniqueInstance(const std::string & lookup_nam } template -template, bool>> +template +requires InterfaceConstructible T * ClassLoader::createUnmanagedInstance(const std::string & lookup_name, Args &&... args) /***************************************************************************/ { @@ -239,11 +241,9 @@ std::map ClassLoader::determineAvailableClasses( std::map classes_available; // Walk the list of all plugin XML files (variable "paths") that are exported by the build system - for (std::vector::const_iterator it = plugin_xml_paths.begin(); - it != plugin_xml_paths.end(); ++it) - { + for (const auto & xml_path : plugin_xml_paths) { try { - processSingleXMLPluginFile(*it, classes_available); + processSingleXMLPluginFile(xml_path, classes_available); } catch (const pluginlib::InvalidXMLException & e) { RCUTILS_LOG_ERROR_NAMED("pluginlib.ClassLoader", "Skipped loading plugin with error: %s.", @@ -447,12 +447,12 @@ std::string ClassLoader::getClassLibraryPath(const std::string & lookup_name) RCUTILS_LOG_DEBUG_NAMED("pluginlib.ClassLoader", "Iterating through all possible paths where %s could be located...", library_name.c_str()); - for (auto path_it = paths_to_try.begin(); path_it != paths_to_try.end(); path_it++) { - RCUTILS_LOG_DEBUG_NAMED("pluginlib.ClassLoader", "Checking path %s ", path_it->c_str()); - if (std::filesystem::exists(*path_it)) { + for (const auto & path : paths_to_try) { + RCUTILS_LOG_DEBUG_NAMED("pluginlib.ClassLoader", "Checking path %s ", path.c_str()); + if (std::filesystem::exists(path)) { RCUTILS_LOG_DEBUG_NAMED("pluginlib.ClassLoader", "Library %s found at explicit path %s.", - library_name.c_str(), path_it->c_str()); - return *path_it; + library_name.c_str(), path.c_str()); + return path; } } std::ostringstream error_msg; @@ -483,12 +483,8 @@ template std::vector ClassLoader::getDeclaredClasses() /***************************************************************************/ { - std::vector lookup_names; - for (ClassMapIterator it = classes_available_.begin(); it != classes_available_.end(); ++it) { - lookup_names.push_back(it->first); - } - - return lookup_names; + auto keys = classes_available_ | std::views::keys; + return {keys.begin(), keys.end()}; } template @@ -496,9 +492,8 @@ std::string ClassLoader::getErrorStringForUnknownClass(const std::string & lo /***************************************************************************/ { std::string declared_types; - std::vector types = getDeclaredClasses(); - for (unsigned int i = 0; i < types.size(); i++) { - declared_types = declared_types + std::string(" ") + types[i]; + for (const auto & type : getDeclaredClasses()) { + declared_types += ' ' + type; } return "According to the loaded plugin descriptions the class " + lookup_name + " with base class type " + base_class_ + " does not exist. Declared types are " + @@ -598,7 +593,7 @@ template bool ClassLoader::isClassAvailable(const std::string & lookup_name) /***************************************************************************/ { - return classes_available_.find(lookup_name) != classes_available_.end(); + return classes_available_.contains(lookup_name); } template @@ -659,16 +654,15 @@ void ClassLoader::processSingleXMLPluginFile( "' has an invalid Root Element. This likely means the XML is malformed or missing."); return; } - if (!(strcmp(config_value, "library") == 0 || - strcmp(config_value, "class_libraries") == 0)) - { + const std::string_view root_tag{config_value}; + if (root_tag != "library" && root_tag != "class_libraries") { throw pluginlib::InvalidXMLException( "The XML document '" + xml_file + "' given to add must have either \"library\" or " "\"class_libraries\" as the root tag"); return; } // Step into the filter list if necessary - if (strcmp(config_value, "class_libraries") == 0) { + if (root_tag == "class_libraries") { config = config->FirstChildElement("library"); } @@ -739,9 +733,11 @@ void ClassLoader::processSingleXMLPluginFile( description_str = "No 'description' tag for this plugin in plugin description file."; } - classes_available.insert(std::pair(lookup_name, - ClassDesc(lookup_name, derived_class, base_class_type, package_name, description_str, - library_path, xml_file))); + // try_emplace keeps the first entry for a duplicate lookup name, matching the + // previous insert(), but builds the ClassDesc only when the key is new. + classes_available.try_emplace(lookup_name, + lookup_name, derived_class, base_class_type, package_name, description_str, + library_path, xml_file); } // step to next class_element @@ -757,14 +753,13 @@ void ClassLoader::refreshDeclaredClasses() { RCUTILS_LOG_DEBUG_NAMED("pluginlib.ClassLoader", "Refreshing declared classes."); // determine classes not currently loaded for removal + // The registered library list is fixed for the duration of this scan, so query it once + // rather than rebuilding the vector for every declared class. + const std::vector open_libs = lowlevel_class_loader_.getRegisteredLibraries(); std::list remove_classes; - for (std::map::const_iterator it = classes_available_.begin(); - it != classes_available_.end(); it++) - { - std::string resolved_library_path = it->second.resolved_library_path_; - std::vector open_libs = lowlevel_class_loader_.getRegisteredLibraries(); - if (std::find(open_libs.begin(), open_libs.end(), resolved_library_path) != open_libs.end()) { - remove_classes.push_back(it->first); + for (const auto & [lookup_name, desc] : classes_available_) { + if (std::ranges::find(open_libs, desc.resolved_library_path_) != open_libs.end()) { + remove_classes.push_back(lookup_name); } } @@ -776,13 +771,9 @@ void ClassLoader::refreshDeclaredClasses() // add new classes plugin_xml_paths_ = getPluginXmlPaths(package_, attrib_name_); std::map updated_classes = determineAvailableClasses(plugin_xml_paths_); - for (std::map::const_iterator it = updated_classes.begin(); - it != updated_classes.end(); it++) - { - if (classes_available_.find(it->first) == classes_available_.end()) { - classes_available_.insert(std::pair(it->first, it->second)); - } - } + // Range insert keeps any entry already present, which is what the previous + // contains()-guarded insert did one lookup at a time. + classes_available_.insert(updated_classes.begin(), updated_classes.end()); } template