From 4d00e99c97b0f8877505347fd46a505fe96868a0 Mon Sep 17 00:00:00 2001 From: Simon Spannagel Date: Wed, 9 Jun 2021 15:48:45 +0200 Subject: [PATCH 1/6] Configuration: port unused key warnings --- src/core/config/ConfigManager.cpp | 14 ++---- src/core/config/ConfigManager.hpp | 5 +-- src/core/config/ConfigReader.cpp | 2 +- src/core/config/Configuration.cpp | 45 ++++++++++++++++++-- src/core/config/Configuration.hpp | 71 +++++++++++++++++++++++++++++-- src/core/config/Configuration.tpp | 16 ++++++- src/core/config/OptionParser.hpp | 4 +- src/core/config/exceptions.cpp | 2 +- 8 files changed, 133 insertions(+), 26 deletions(-) diff --git a/src/core/config/ConfigManager.cpp b/src/core/config/ConfigManager.cpp index 6cfae0fe..ea44d238 100644 --- a/src/core/config/ConfigManager.cpp +++ b/src/core/config/ConfigManager.cpp @@ -101,7 +101,7 @@ bool ConfigManager::loadModuleOptions(const std::vector& options) { bool optionsApplied = false; // Parse the options - for(auto& option : options) { + for(const auto& option : options) { module_option_parser_.parseOption(option); } @@ -128,15 +128,12 @@ bool ConfigManager::loadDetectorOptions(const std::vector& options) OptionParser detector_option_parser; // Parse the options - for(auto& option : options) { + for(const auto& option : options) { detector_option_parser.parseOption(option); } - if(detector_configs_.empty()) { - parse_detectors(); - } - // Apply detector options + parse_detectors(); for(auto& config : detector_configs_) { optionsApplied = detector_option_parser.applyOptions(config.getName(), config) || optionsApplied; } @@ -154,10 +151,7 @@ std::list& ConfigManager::getModuleConfigurations() { * The list of detector configurations is read from the configuration defined in 'detector_file' */ std::list& ConfigManager::getDetectorConfigurations() { - if(detector_configs_.empty()) { - parse_detectors(); - } - + parse_detectors(); return detector_configs_; } diff --git a/src/core/config/ConfigManager.hpp b/src/core/config/ConfigManager.hpp index a9e24743..a8d8d29f 100644 --- a/src/core/config/ConfigManager.hpp +++ b/src/core/config/ConfigManager.hpp @@ -114,14 +114,13 @@ namespace corryvreckan { std::set global_names_{}; std::set ignore_names_{}; - // Helper function to parse detectors file - void parse_detectors(); - OptionParser module_option_parser_; std::list module_configs_; Configuration global_config_; + // Helper function for delayed parsing of detectors file + void parse_detectors(); std::list detector_configs_; std::list instance_configs_; diff --git a/src/core/config/ConfigReader.cpp b/src/core/config/ConfigReader.cpp index b71cf61e..4cb5a9b5 100644 --- a/src/core/config/ConfigReader.cpp +++ b/src/core/config/ConfigReader.cpp @@ -230,7 +230,7 @@ std::vector ConfigReader::getConfigurations(std::string name) con } std::vector result; - for(auto& iter : conf_map_.at(name)) { + for(const auto& iter : conf_map_.at(name)) { result.push_back(*iter); } return result; diff --git a/src/core/config/Configuration.cpp b/src/core/config/Configuration.cpp index 80f3568d..e63cf7ef 100644 --- a/src/core/config/Configuration.cpp +++ b/src/core/config/Configuration.cpp @@ -20,6 +20,25 @@ using namespace corryvreckan; +Configuration::AccessMarker::AccessMarker(const Configuration::AccessMarker& rhs) { + for(const auto& [key, value] : rhs.markers_) { + registerMarker(key); + markers_.at(key).store(value.load()); + } +} + +Configuration::AccessMarker& Configuration::AccessMarker::operator=(const Configuration::AccessMarker& rhs) { + for(const auto& [key, value] : rhs.markers_) { + registerMarker(key); + markers_.at(key).store(value.load()); + } + return *this; +} + +void Configuration::AccessMarker::registerMarker(const std::string& key) { + markers_.emplace(std::piecewise_construct, std::forward_as_tuple(key), std::forward_as_tuple()); +} + Configuration::Configuration(std::string name, std::string path) : name_(std::move(name)), path_(std::move(path)) {} bool Configuration::has(const std::string& key) const { @@ -32,7 +51,7 @@ unsigned int Configuration::count(std::initializer_list keys) const } unsigned int found = 0; - for(auto& key : keys) { + for(const auto& key : keys) { if(has(key)) { found++; } @@ -50,6 +69,7 @@ std::string Configuration::getFilePath() const { std::string Configuration::getText(const std::string& key) const { try { // NOTE: returning literally including "" + used_keys_.markUsed(key); return config_.at(key); } catch(std::out_of_range& e) { throw MissingKeyError(key, getName()); @@ -117,10 +137,11 @@ std::string Configuration::path_to_absolute(std::string path, bool canonicalize_ void Configuration::setText(const std::string& key, const std::string& val) { config_[key] = val; + used_keys_.registerMarker(key); } /** - * The alias is only used if new key does not exist but old key does + * The alias is only used if new key does not exist but old key does. The old key is automatically marked as used. */ void Configuration::setAlias(const std::string& new_key, const std::string& old_key, bool warn) { if(!has(old_key) || has(new_key)) { @@ -128,6 +149,8 @@ void Configuration::setAlias(const std::string& new_key, const std::string& old_ } try { config_[new_key] = config_.at(old_key); + used_keys_.registerMarker(new_key); + used_keys_.markUsed(old_key); } catch(std::out_of_range& e) { throw MissingKeyError(old_key, getName()); } @@ -153,11 +176,11 @@ void Configuration::merge(const Configuration& other) { } } -std::vector> Configuration::getAll() { +std::vector> Configuration::getAll() const { std::vector> result; // Loop over all configuration keys - for(auto& key_value : config_) { + for(const auto& key_value : config_) { // Skip internal keys starting with an underscore if(!key_value.first.empty() && key_value.first.front() == '_') { continue; @@ -169,6 +192,20 @@ std::vector> Configuration::getAll() { return result; } +std::vector Configuration::getUnusedKeys() const { + std::vector result; + + // Loop over all configuration keys, excluding internal ones + for(const auto& key_value : getAll()) { + // Add those to result that have not been accessed: + if(!used_keys_.isUsed(key_value.first)) { + result.emplace_back(key_value.first); + } + } + + return result; +} + /** * String is recursively parsed for all pair of [ and ] brackets. All parts between single or double quotation marks are * skipped. diff --git a/src/core/config/Configuration.hpp b/src/core/config/Configuration.hpp index 629ff6af..f3ec6ef4 100644 --- a/src/core/config/Configuration.hpp +++ b/src/core/config/Configuration.hpp @@ -10,6 +10,7 @@ #ifndef CORRYVRECKAN_CONFIGURATION_H #define CORRYVRECKAN_CONFIGURATION_H +#include #include #include #include @@ -31,6 +32,58 @@ namespace corryvreckan { * the library of \ref StringConversions. */ class Configuration { + + /** + * @brief Helper class to keep track of key access + * + * This class holds all configuration keys in a map together with an atomic boolean marking whether they have been + * accessed already. This allows to find out whick keys have not been accessed at all. This wrapper allows to use + * atomics for non-locking access but requires to register all keys beforehand. + */ + class AccessMarker { + public: + /** + * Default constructor + */ + AccessMarker() = default; + + /** + * @brief Explicit copy constructor to allow copying of the map keys + */ + AccessMarker(const AccessMarker& rhs); + + /** + * @brief Explicit copy assignment operator to allow copying of the map keys + */ + AccessMarker& operator=(const AccessMarker& rhs); + + /** + * @brief Method to register a key for a new access marker + * @param key Key of the marker + * @warning This operation is not thread-safe + */ + void registerMarker(const std::string& key); + + /** + * @brief Method to mark existing marker as accessed/used. + * @param key Key of the marker + * @note This is an atomic operation and thread-safe. + * @throws std::out_of_range if the key has not been registered beforehand + */ + void markUsed(const std::string& key) { markers_.at(key).store(true); }; + + /** + * @brief Method to retrieve access status of an existing marker. + * @param key Key of the marker + * @note This is an atomic operation and thread-safe. + * @throws std::out_of_range if the key has not been registered beforehand + */ + bool isUsed(const std::string& key) { return markers_.at(key).load(); } + + private: + std::map> markers_; + }; + public: /** * @brief Construct a configuration object @@ -153,8 +206,9 @@ namespace corryvreckan { * @brief Set value for a key in a given type * @param key Key to set value of * @param val Value to assign to the key + * @param mark_used Flag whether key should be marked as "used" directly */ - template void set(const std::string& key, const T& val); + template void set(const std::string& key, const T& val, bool mark_used = false); /** * @brief Store value for a key in a given type, including units @@ -168,9 +222,10 @@ namespace corryvreckan { * @brief Set list of values for a key in a given type * @param key Key to set values of * @param val List of values to assign to the key + * @param mark_used Flag whether key should be marked as "used" directly */ // TODO [doc] Provide second template parameter to specify the vector type to return it in - template void setArray(const std::string& key, const std::vector& val); + template void setArray(const std::string& key, const std::vector& val, bool mark_used = false); /** * @brief Set matrix of values for a key in a given type @@ -211,6 +266,7 @@ namespace corryvreckan { * @param new_key New alias to be created * @param old_key Key the alias is created for * @param warn Optionally print a warning message to notify of deprecation + * @note This marks the old key as "used" automatically */ void setAlias(const std::string& new_key, const std::string& old_key, bool warn = false); @@ -245,7 +301,15 @@ namespace corryvreckan { * @return List of all key value pairs */ // FIXME Better name for this function - std::vector> getAll(); + std::vector> getAll() const; + + /** + * @brief Obtain all keys which have not been accessed yet + * + * This method returns all keys from the configuration object which have not yet been accessed, Default values as + * well as aliases are marked as used automatically and are therefore never returned. + */ + std::vector getUnusedKeys() const; private: /** @@ -275,6 +339,7 @@ namespace corryvreckan { using ConfigMap = std::map; ConfigMap config_; + mutable AccessMarker used_keys_; }; } // namespace corryvreckan diff --git a/src/core/config/Configuration.tpp b/src/core/config/Configuration.tpp index df4494b1..ce7d0e87 100644 --- a/src/core/config/Configuration.tpp +++ b/src/core/config/Configuration.tpp @@ -16,6 +16,7 @@ namespace corryvreckan { template T Configuration::get(const std::string& key) const { try { auto node = parse_value(config_.at(key)); + used_keys_.markUsed(key); try { return corryvreckan::from_string(node->value); } catch(std::invalid_argument& e) { @@ -48,6 +49,7 @@ namespace corryvreckan { template std::vector Configuration::getArray(const std::string& key) const { try { std::string str = config_.at(key); + used_keys_.markUsed(key); std::vector array; auto node = parse_value(str); @@ -86,6 +88,7 @@ namespace corryvreckan { template Matrix Configuration::getMatrix(const std::string& key) const { try { std::string str = config_.at(key); + used_keys_.markUsed(key); Matrix matrix; auto node = parse_value(str); @@ -126,8 +129,12 @@ namespace corryvreckan { return def; } - template void Configuration::set(const std::string& key, const T& val) { + template void Configuration::set(const std::string& key, const T& val, bool mark_used) { config_[key] = corryvreckan::to_string(val); + used_keys_.registerMarker(key); + if(mark_used) { + used_keys_.markUsed(key); + } } template @@ -141,9 +148,10 @@ namespace corryvreckan { } ret_str.pop_back(); config_[key] = ret_str; + used_keys_.registerMarker(key); } - template void Configuration::setArray(const std::string& key, const std::vector& val) { + template void Configuration::setArray(const std::string& key, const std::vector& val, bool mark_used) { // NOTE: not the most elegant way to support arrays std::string str; for(T el : val) { @@ -152,6 +160,10 @@ namespace corryvreckan { } str.pop_back(); config_[key] = str; + used_keys_.registerMarker(key); + if(mark_used) { + used_keys_.markUsed(key); + } } template void Configuration::setMatrix(const std::string& key, const Matrix& val) { diff --git a/src/core/config/OptionParser.hpp b/src/core/config/OptionParser.hpp index 4cdbdb4a..63f02d4b 100644 --- a/src/core/config/OptionParser.hpp +++ b/src/core/config/OptionParser.hpp @@ -48,8 +48,8 @@ namespace corryvreckan { /** * @brief Use default move behaviour */ - OptionParser(OptionParser&&) noexcept = default; - OptionParser& operator=(OptionParser&&) noexcept = default; + OptionParser(OptionParser&&) noexcept = default; // NOLINT + OptionParser& operator=(OptionParser&&) = default; /// @} /** diff --git a/src/core/config/exceptions.cpp b/src/core/config/exceptions.cpp index 86c4267c..24e8e0a0 100644 --- a/src/core/config/exceptions.cpp +++ b/src/core/config/exceptions.cpp @@ -32,7 +32,7 @@ InvalidCombinationError::InvalidCombinationError(const Configuration& config, section_str = "in global section"; } error_message_ = "Combination of keys "; - for(auto& key : keys) { + for(const auto& key : keys) { if(!config.has(key)) { continue; } -- GitLab From e66a9e9640c6d43978ce29ced2574392e73f119a Mon Sep 17 00:00:00 2001 From: Simon Spannagel Date: Wed, 9 Jun 2021 16:23:54 +0200 Subject: [PATCH 2/6] ModuleManager: print unused keys --- src/core/module/ModuleManager.cpp | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/core/module/ModuleManager.cpp b/src/core/module/ModuleManager.cpp index 3df3cd43..029ccea3 100644 --- a/src/core/module/ModuleManager.cpp +++ b/src/core/module/ModuleManager.cpp @@ -750,6 +750,34 @@ void ModuleManager::finalizeAll() { LOG(STATUS) << "Wrote updated detector configuration to " << path; } + // Check for unused configuration keys: + auto unused_keys = global_config.getUnusedKeys(); + if(!unused_keys.empty()) { + std::stringstream st; + st << "Unused configuration keys in global section:"; + for(auto& key : unused_keys) { + st << std::endl << key; + } + LOG(WARNING) << st.str(); + } + for(auto& config : conf_manager_->getInstanceConfigurations()) { + auto unique_name = config.getName(); + auto identifier = config.get("identifier"); + if(!identifier.empty()) { + unique_name += ":"; + unique_name += identifier; + } + auto cfg_unused_keys = config.getUnusedKeys(); + if(!cfg_unused_keys.empty()) { + std::stringstream st; + st << "Unused configuration keys in section " << unique_name << ":"; + for(auto& key : cfg_unused_keys) { + st << std::endl << key; + } + LOG(WARNING) << st.str(); + } + } + // Check the timing for all events timing(); } -- GitLab From 03f7bcb8577418a8daefb7eb1874f956b7dd0f58 Mon Sep 17 00:00:00 2001 From: Simon Spannagel Date: Wed, 9 Jun 2021 16:29:09 +0200 Subject: [PATCH 3/6] Config: only parse if necessary --- src/core/config/ConfigManager.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/core/config/ConfigManager.cpp b/src/core/config/ConfigManager.cpp index ea44d238..d5137f4b 100644 --- a/src/core/config/ConfigManager.cpp +++ b/src/core/config/ConfigManager.cpp @@ -132,8 +132,11 @@ bool ConfigManager::loadDetectorOptions(const std::vector& options) detector_option_parser.parseOption(option); } + if(detector_configs_.empty()) { + parse_detectors(); + } + // Apply detector options - parse_detectors(); for(auto& config : detector_configs_) { optionsApplied = detector_option_parser.applyOptions(config.getName(), config) || optionsApplied; } @@ -151,7 +154,9 @@ std::list& ConfigManager::getModuleConfigurations() { * The list of detector configurations is read from the configuration defined in 'detector_file' */ std::list& ConfigManager::getDetectorConfigurations() { - parse_detectors(); + if(detector_configs_.empty()) { + parse_detectors(); + } return detector_configs_; } -- GitLab From 3d61ef06123f4d1de304f7e07e3a9a21709c25c3 Mon Sep 17 00:00:00 2001 From: Simon Spannagel Date: Wed, 9 Jun 2021 16:31:55 +0200 Subject: [PATCH 4/6] Corry: set version as used key --- src/core/Corryvreckan.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/Corryvreckan.cpp b/src/core/Corryvreckan.cpp index 515f1aaa..3507da58 100644 --- a/src/core/Corryvreckan.cpp +++ b/src/core/Corryvreckan.cpp @@ -109,7 +109,7 @@ void Corryvreckan::load() { // Put welcome message and set version LOG(STATUS) << "Welcome to Corryvreckan " << CORRYVRECKAN_PROJECT_VERSION; - global_config.set("version", CORRYVRECKAN_PROJECT_VERSION); + global_config.set("version", CORRYVRECKAN_PROJECT_VERSION, true); // Get output directory std::string directory = gSystem->pwd(); -- GitLab From fdbbbaf86b392560aff9f5ca323e9225691eecbd Mon Sep 17 00:00:00 2001 From: Simon Spannagel Date: Wed, 15 Sep 2021 14:19:00 +0200 Subject: [PATCH 5/6] Avoid casting to bool --- src/core/detector/PixelDetector.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/detector/PixelDetector.cpp b/src/core/detector/PixelDetector.cpp index d83cab6a..b4cf7df8 100644 --- a/src/core/detector/PixelDetector.cpp +++ b/src/core/detector/PixelDetector.cpp @@ -186,10 +186,10 @@ void PixelDetector::configure_detector(Configuration& config) const { config.set("number_of_pixels", m_nPixels); // Size of the pixels - config.set("pixel_pitch", m_pitch, {"um"}); + config.set("pixel_pitch", m_pitch, {{"um"}}); // Intrinsic resolution: - config.set("spatial_resolution", m_spatial_resolution, {"um"}); + config.set("spatial_resolution", m_spatial_resolution, {{"um"}}); // Pixel mask file: if(!m_maskfile.empty()) { @@ -207,7 +207,7 @@ void PixelDetector::configure_detector(Configuration& config) const { void PixelDetector::configure_pos_and_orientation(Configuration& config) const { config.set("position", m_displacement, {"um", "mm"}); config.set("orientation_mode", m_orientation_mode); - config.set("orientation", m_orientation, {"deg"}); + config.set("orientation", m_orientation, {{"deg"}}); } // Function to get global intercept with a track -- GitLab From 46fcd2058557b0a855c27e1605049800a48ca752 Mon Sep 17 00:00:00 2001 From: Simon Spannagel Date: Wed, 15 Sep 2021 14:24:02 +0200 Subject: [PATCH 6/6] Don't require C++17 for this change (yet!) --- src/core/config/Configuration.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/core/config/Configuration.cpp b/src/core/config/Configuration.cpp index e63cf7ef..2b8f6891 100644 --- a/src/core/config/Configuration.cpp +++ b/src/core/config/Configuration.cpp @@ -21,16 +21,16 @@ using namespace corryvreckan; Configuration::AccessMarker::AccessMarker(const Configuration::AccessMarker& rhs) { - for(const auto& [key, value] : rhs.markers_) { - registerMarker(key); - markers_.at(key).store(value.load()); + for(const auto& key : rhs.markers_) { + registerMarker(key.first); + markers_.at(key.first).store(key.second.load()); } } Configuration::AccessMarker& Configuration::AccessMarker::operator=(const Configuration::AccessMarker& rhs) { - for(const auto& [key, value] : rhs.markers_) { - registerMarker(key); - markers_.at(key).store(value.load()); + for(const auto& key : rhs.markers_) { + registerMarker(key.first); + markers_.at(key.first).store(key.second.load()); } return *this; } -- GitLab