From a6319346eed5bdbaea688c2ae7eaa146bb79c013 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ale=C5=A1=20Mat=C4=9Bj?= Date: Thu, 4 Apr 2024 06:23:38 +0200 Subject: [PATCH 1/5] Add pImpl to `libdnf5::Config` --- include/libdnf5/conf/config.hpp | 8 +++++--- libdnf5/conf/config.cpp | 18 ++++++++++++++++-- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/include/libdnf5/conf/config.hpp b/include/libdnf5/conf/config.hpp index a0d004a36..e5faf2746 100644 --- a/include/libdnf5/conf/config.hpp +++ b/include/libdnf5/conf/config.hpp @@ -33,9 +33,10 @@ namespace libdnf5 { /// Base class for configurations objects class Config { public: - OptionBinds & opt_binds() noexcept { return binds; } + OptionBinds & opt_binds() noexcept; - virtual ~Config() = default; + Config(); + virtual ~Config(); virtual void load_from_parser( const ConfigParser & parser, @@ -45,7 +46,8 @@ class Config { Option::Priority priority); private: - OptionBinds binds; + class Impl; + ImplPtr p_impl; }; diff --git a/libdnf5/conf/config.cpp b/libdnf5/conf/config.cpp index f923a21e3..e93850cf7 100644 --- a/libdnf5/conf/config.cpp +++ b/libdnf5/conf/config.cpp @@ -22,6 +22,20 @@ along with libdnf. If not, see . namespace libdnf5 { +class Config::Impl { +private: + friend Config; + + OptionBinds binds; +}; + +OptionBinds & Config::opt_binds() noexcept { + return p_impl->binds; +} + +Config::~Config() = default; +Config::Config() : p_impl(new Impl()) {} + void Config::load_from_parser( const ConfigParser & parser, const std::string & section, @@ -31,8 +45,8 @@ void Config::load_from_parser( auto cfg_parser_data_iter = parser.get_data().find(section); if (cfg_parser_data_iter != parser.get_data().end()) { for (const auto & opt : cfg_parser_data_iter->second) { - auto opt_binds_iter = binds.find(opt.first); - if (opt_binds_iter != binds.end()) { + auto opt_binds_iter = p_impl->binds.find(opt.first); + if (opt_binds_iter != p_impl->binds.end()) { try { opt_binds_iter->second.new_string(priority, vars.substitute(opt.second)); } catch (const OptionError & ex) { From b4f81989e8c604e84191da9a49d115b038f189b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ale=C5=A1=20Mat=C4=9Bj?= Date: Thu, 4 Apr 2024 06:40:04 +0200 Subject: [PATCH 2/5] Add pImpl to `libdnf5::OptionBinds::Item` --- include/libdnf5/conf/option_binds.hpp | 8 ++--- libdnf5/conf/option_binds.cpp | 42 ++++++++++++++++++--------- 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/include/libdnf5/conf/option_binds.hpp b/include/libdnf5/conf/option_binds.hpp index 0c5f7bbd2..6958b1386 100644 --- a/include/libdnf5/conf/option_binds.hpp +++ b/include/libdnf5/conf/option_binds.hpp @@ -57,6 +57,7 @@ class OptionBinds { using NewStringFunc = std::function; using GetValueStringFunc = std::function; + ~Item(); Option::Priority get_priority() const; void new_string(Option::Priority priority, const std::string & value); std::string get_value_string() const; @@ -67,10 +68,9 @@ class OptionBinds { Item(Option & option, NewStringFunc new_string_func, GetValueStringFunc get_value_string_func, bool add_value); explicit Item(Option & option); - Option * option; - NewStringFunc new_str_func; - GetValueStringFunc get_value_str_func; - bool is_append_option{false}; // hint that new value be added/appended + + class Impl; + ImplPtr p_impl; }; using Container = std::map; diff --git a/libdnf5/conf/option_binds.cpp b/libdnf5/conf/option_binds.cpp index 57dbf1b00..f60bae9e8 100644 --- a/libdnf5/conf/option_binds.cpp +++ b/libdnf5/conf/option_binds.cpp @@ -32,37 +32,53 @@ OptionBindsOptionAlreadyExistsError::OptionBindsOptionAlreadyExistsError(const s : OptionBindsError(M_("Option \"{}\" already exists"), id) {} // ========== OptionBinds::Item class =============== +class OptionBinds::Item::Impl { +public: + Impl(Option & option, NewStringFunc && new_string_func, GetValueStringFunc && get_value_string_func, bool add_value) + : option(&option), + new_str_func(std::move(new_string_func)), + get_value_str_func(std::move(get_value_string_func)), + is_append_option(add_value) {} + + Impl(Option & option) : option(&option){}; + +private: + friend OptionBinds::Item; + Option * option; + NewStringFunc new_str_func; + GetValueStringFunc get_value_str_func; + bool is_append_option{false}; // hint that new value be added/appended +}; OptionBinds::Item::Item( Option & option, NewStringFunc new_string_func, GetValueStringFunc get_value_string_func, bool add_value) - : option(&option), - new_str_func(std::move(new_string_func)), - get_value_str_func(std::move(get_value_string_func)), - is_append_option(add_value) {} + : p_impl(new Impl(option, std::move(new_string_func), std::move(get_value_string_func), add_value)) {} -OptionBinds::Item::Item(Option & option) : option(&option) {} +OptionBinds::Item::Item(Option & option) : p_impl(new Impl(option)) {} + +OptionBinds::Item::~Item() = default; Option::Priority OptionBinds::Item::get_priority() const { - return option->get_priority(); + return p_impl->option->get_priority(); } void OptionBinds::Item::new_string(Option::Priority priority, const std::string & value) { - if (new_str_func) { - new_str_func(priority, value); + if (p_impl->new_str_func) { + p_impl->new_str_func(priority, value); } else { - option->set(priority, value); + p_impl->option->set(priority, value); } } std::string OptionBinds::Item::get_value_string() const { - if (get_value_str_func) { - return get_value_str_func(); + if (p_impl->get_value_str_func) { + return p_impl->get_value_str_func(); } - return option->get_value_string(); + return p_impl->option->get_value_string(); } bool OptionBinds::Item::get_is_append_option() const { - return is_append_option; + return p_impl->is_append_option; } From aba25f308d03e62a20efd1f7bcb3c79e75be290b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ale=C5=A1=20Mat=C4=9Bj?= Date: Thu, 4 Apr 2024 06:40:28 +0200 Subject: [PATCH 3/5] Add pImpl to `libdnf5::OptionBinds` --- include/libdnf5/conf/option_binds.hpp | 27 +++++++----- libdnf5/conf/option_binds.cpp | 62 ++++++++++++++++++++++----- 2 files changed, 68 insertions(+), 21 deletions(-) diff --git a/include/libdnf5/conf/option_binds.hpp b/include/libdnf5/conf/option_binds.hpp index 6958b1386..9f2e832e7 100644 --- a/include/libdnf5/conf/option_binds.hpp +++ b/include/libdnf5/conf/option_binds.hpp @@ -77,6 +77,10 @@ class OptionBinds { using iterator = Container::iterator; using const_iterator = Container::const_iterator; + OptionBinds(); + OptionBinds(const OptionBinds & src); + ~OptionBinds(); + Item & add( const std::string & id, Option & option, @@ -86,19 +90,20 @@ class OptionBinds { Item & add(const std::string & id, Option & option); Item & at(const std::string & id); const Item & at(const std::string & id) const; - bool empty() const noexcept { return items.empty(); } - std::size_t size() const noexcept { return items.size(); } - iterator begin() noexcept { return items.begin(); } - const_iterator begin() const noexcept { return items.begin(); } - const_iterator cbegin() const noexcept { return items.cbegin(); } - iterator end() noexcept { return items.end(); } - const_iterator end() const noexcept { return items.end(); } - const_iterator cend() const noexcept { return items.cend(); } - iterator find(const std::string & id) { return items.find(id); } - const_iterator find(const std::string & id) const { return items.find(id); } + bool empty() const noexcept; + std::size_t size() const noexcept; + iterator begin() noexcept; + const_iterator begin() const noexcept; + const_iterator cbegin() const noexcept; + iterator end() noexcept; + const_iterator end() const noexcept; + const_iterator cend() const noexcept; + iterator find(const std::string & id); + const_iterator find(const std::string & id) const; private: - Container items; + class Impl; + ImplPtr p_impl; }; } // namespace libdnf5 diff --git a/libdnf5/conf/option_binds.cpp b/libdnf5/conf/option_binds.cpp index f60bae9e8..b2547e4f5 100644 --- a/libdnf5/conf/option_binds.cpp +++ b/libdnf5/conf/option_binds.cpp @@ -83,18 +83,28 @@ bool OptionBinds::Item::get_is_append_option() const { // =========== OptionBinds class =============== +class OptionBinds::Impl { +private: + friend OptionBinds; + + Container items; +}; + +OptionBinds::OptionBinds() : p_impl(new Impl()) {} +OptionBinds::OptionBinds(const OptionBinds & src) = default; +OptionBinds::~OptionBinds() = default; OptionBinds::Item & OptionBinds::at(const std::string & id) { - auto item = items.find(id); - if (item == items.end()) { + auto item = p_impl->items.find(id); + if (item == p_impl->items.end()) { throw OptionBindsOptionNotFoundError(id); } return item->second; } const OptionBinds::Item & OptionBinds::at(const std::string & id) const { - auto item = items.find(id); - if (item == items.end()) { + auto item = p_impl->items.find(id); + if (item == p_impl->items.end()) { throw OptionBindsOptionNotFoundError(id); } return item->second; @@ -106,21 +116,53 @@ OptionBinds::Item & OptionBinds::add( Item::NewStringFunc new_string_func, Item::GetValueStringFunc get_value_string_func, bool add_value) { - auto item = items.find(id); - if (item != items.end()) { + auto item = p_impl->items.find(id); + if (item != p_impl->items.end()) { throw OptionBindsOptionAlreadyExistsError(id); } - auto res = items.emplace(id, Item(option, std::move(new_string_func), std::move(get_value_string_func), add_value)); + auto res = p_impl->items.emplace( + id, Item(option, std::move(new_string_func), std::move(get_value_string_func), add_value)); return res.first->second; } OptionBinds::Item & OptionBinds::add(const std::string & id, Option & option) { - auto item = items.find(id); - if (item != items.end()) { + auto item = p_impl->items.find(id); + if (item != p_impl->items.end()) { throw OptionBindsOptionAlreadyExistsError(id); } - auto res = items.emplace(id, Item(option)); + auto res = p_impl->items.emplace(id, Item(option)); return res.first->second; } +bool OptionBinds::empty() const noexcept { + return p_impl->items.empty(); +} +std::size_t OptionBinds::size() const noexcept { + return p_impl->items.size(); +} +OptionBinds::iterator OptionBinds::begin() noexcept { + return p_impl->items.begin(); +} +OptionBinds::const_iterator OptionBinds::begin() const noexcept { + return p_impl->items.begin(); +} +OptionBinds::const_iterator OptionBinds::cbegin() const noexcept { + return p_impl->items.cbegin(); +} +OptionBinds::iterator OptionBinds::end() noexcept { + return p_impl->items.end(); +} +OptionBinds::const_iterator OptionBinds::end() const noexcept { + return p_impl->items.end(); +} +OptionBinds::const_iterator OptionBinds::cend() const noexcept { + return p_impl->items.cend(); +} +OptionBinds::iterator OptionBinds::find(const std::string & id) { + return p_impl->items.find(id); +} +OptionBinds::const_iterator OptionBinds::find(const std::string & id) const { + return p_impl->items.find(id); +} + } // namespace libdnf5 From 5dbaf5e285c06e5ca75ed07a34f33bac52db6d35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ale=C5=A1=20Mat=C4=9Bj?= Date: Thu, 4 Apr 2024 11:28:37 +0200 Subject: [PATCH 4/5] Add pImpl to `libdnf5::MemoryBufferLogger` --- .../libdnf5/logger/memory_buffer_logger.hpp | 13 ++-- libdnf5/logger/memory_buffer_logger.cpp | 69 ++++++++++++------- 2 files changed, 48 insertions(+), 34 deletions(-) diff --git a/include/libdnf5/logger/memory_buffer_logger.hpp b/include/libdnf5/logger/memory_buffer_logger.hpp index 479ebbb35..8516c2f46 100644 --- a/include/libdnf5/logger/memory_buffer_logger.hpp +++ b/include/libdnf5/logger/memory_buffer_logger.hpp @@ -22,9 +22,7 @@ along with libdnf. If not, see . #include "logger.hpp" -#include -#include - +#include "libdnf5/common/impl_ptr.hpp" namespace libdnf5 { @@ -40,6 +38,7 @@ class MemoryBufferLogger : public Logger { }; explicit MemoryBufferLogger(std::size_t max_items_to_keep, std::size_t reserve = 0); + ~MemoryBufferLogger(); void write( const std::chrono::time_point & time, @@ -47,16 +46,14 @@ class MemoryBufferLogger : public Logger { Level level, const std::string & message) noexcept override; - std::size_t get_items_count() const { return items.size(); } + std::size_t get_items_count() const; const Item & get_item(std::size_t item_idx) const; void clear() noexcept; void write_to_logger(Logger & logger); private: - mutable std::mutex items_mutex; - std::size_t max_items; // rotation, oldest messages are replaced - std::size_t first_item_idx; - std::vector items; + class Impl; + ImplPtr p_impl; }; } // namespace libdnf5 diff --git a/libdnf5/logger/memory_buffer_logger.cpp b/libdnf5/logger/memory_buffer_logger.cpp index 3938aed6c..f743fcb1e 100644 --- a/libdnf5/logger/memory_buffer_logger.cpp +++ b/libdnf5/logger/memory_buffer_logger.cpp @@ -21,14 +21,27 @@ along with libdnf. If not, see . namespace libdnf5 { -MemoryBufferLogger::MemoryBufferLogger(std::size_t max_items_to_keep, std::size_t reserve) - : max_items(max_items_to_keep), - first_item_idx(0) { - if (reserve > 0) { - items.reserve(reserve < max_items_to_keep ? reserve : max_items_to_keep); +class MemoryBufferLogger::Impl { +public: + Impl(std::size_t max_items_to_keep, std::size_t reserve) : max_items(max_items_to_keep), first_item_idx(0) { + if (reserve > 0) { + items.reserve(reserve < max_items_to_keep ? reserve : max_items_to_keep); + } } -} +private: + friend MemoryBufferLogger; + + mutable std::mutex items_mutex; + std::size_t max_items; // rotation, oldest messages are replaced + std::size_t first_item_idx; + std::vector items; +}; + +MemoryBufferLogger::MemoryBufferLogger(std::size_t max_items_to_keep, std::size_t reserve) + : p_impl(new Impl(max_items_to_keep, reserve)) {} + +MemoryBufferLogger::~MemoryBufferLogger() = default; void MemoryBufferLogger::write( const std::chrono::time_point & time, @@ -36,13 +49,13 @@ void MemoryBufferLogger::write( Level level, const std::string & message) noexcept { try { - std::lock_guard guard(items_mutex); - if (max_items == 0 || items.size() < max_items) { - items.push_back({time, pid, level, message}); + std::lock_guard guard(p_impl->items_mutex); + if (p_impl->max_items == 0 || p_impl->items.size() < p_impl->max_items) { + p_impl->items.push_back({time, pid, level, message}); } else { - items[first_item_idx] = {time, pid, level, message}; - if (++first_item_idx >= max_items) { - first_item_idx = 0; + p_impl->items[p_impl->first_item_idx] = {time, pid, level, message}; + if (++p_impl->first_item_idx >= p_impl->max_items) { + p_impl->first_item_idx = 0; } } } catch (...) { @@ -50,34 +63,38 @@ void MemoryBufferLogger::write( } const MemoryBufferLogger::Item & MemoryBufferLogger::get_item(std::size_t item_idx) const { - if (item_idx >= items.size() || item_idx >= max_items) { + if (item_idx >= p_impl->items.size() || item_idx >= p_impl->max_items) { throw std::out_of_range("MemoryBufferLogger"); } - std::lock_guard guard(items_mutex); - auto idx = item_idx + first_item_idx; - if (idx >= max_items) { - idx -= max_items; + std::lock_guard guard(p_impl->items_mutex); + auto idx = item_idx + p_impl->first_item_idx; + if (idx >= p_impl->max_items) { + idx -= p_impl->max_items; } - return items[idx]; + return p_impl->items[idx]; } void MemoryBufferLogger::write_to_logger(Logger & logger) { - std::lock_guard guard(items_mutex); - for (size_t idx = first_item_idx; idx < items.size(); ++idx) { - auto & msg = items[idx]; + std::lock_guard guard(p_impl->items_mutex); + for (size_t idx = p_impl->first_item_idx; idx < p_impl->items.size(); ++idx) { + auto & msg = p_impl->items[idx]; logger.write(msg.time, msg.pid, msg.level, msg.message); } - for (size_t idx = 0; idx < first_item_idx; ++idx) { - auto & msg = items[idx]; + for (size_t idx = 0; idx < p_impl->first_item_idx; ++idx) { + auto & msg = p_impl->items[idx]; logger.write(msg.time, msg.pid, msg.level, msg.message); } } void MemoryBufferLogger::clear() noexcept { - std::lock_guard guard(items_mutex); - first_item_idx = 0; - items.clear(); + std::lock_guard guard(p_impl->items_mutex); + p_impl->first_item_idx = 0; + p_impl->items.clear(); +} + +std::size_t MemoryBufferLogger::get_items_count() const { + return p_impl->items.size(); } } // namespace libdnf5 From d5a9e3e2efa072e91bac7de10f4e54ff1104d387 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ale=C5=A1=20Mat=C4=9Bj?= Date: Thu, 4 Apr 2024 12:05:23 +0200 Subject: [PATCH 5/5] Add pImpl to `libdnf5::LogRouter` --- include/libdnf5/logger/log_router.hpp | 19 ++++++++----- libdnf5/logger/log_router.cpp | 40 ++++++++++++++++++++++++--- 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/include/libdnf5/logger/log_router.hpp b/include/libdnf5/logger/log_router.hpp index a622b6885..a81f3c2ab 100644 --- a/include/libdnf5/logger/log_router.hpp +++ b/include/libdnf5/logger/log_router.hpp @@ -22,6 +22,8 @@ along with libdnf. If not, see . #include "logger.hpp" +#include "libdnf5/common/impl_ptr.hpp" + #include #include @@ -33,26 +35,28 @@ namespace libdnf5 { class LogRouter : public Logger { public: /// Constructs a new LogRouter instance with an empty set of destination loggers. - LogRouter() = default; + LogRouter(); + + ~LogRouter(); /// Constructs a new LogRouter instance and sets the destination loggers. - LogRouter(std::vector> && loggers) : loggers(std::move(loggers)) {} + LogRouter(std::vector> && loggers); /// Moves (registers) the "logger" into the log router. It gets next free index number. - void add_logger(std::unique_ptr && logger) { loggers.push_back(std::move(logger)); } + void add_logger(std::unique_ptr && logger); /// Returns pointer to the logger at the "index" position. - Logger * get_logger(size_t index) { return loggers.at(index).get(); } + Logger * get_logger(size_t index); /// Removes logger at the "index" position from LogRouter. /// The array of the loggers is squeezed. Index of the loggers behind removed logger is decreased by one. std::unique_ptr release_logger(size_t index); /// Swaps the logger at the "index" position with another "logger". - void swap_logger(std::unique_ptr & logger, size_t index) { loggers.at(index).swap(logger); } + void swap_logger(std::unique_ptr & logger, size_t index); /// Returns number of loggers registered in LogRouter. - size_t get_loggers_count() const noexcept { return loggers.size(); } + size_t get_loggers_count() const noexcept; void log_line(Level level, const std::string & message) noexcept override; @@ -63,7 +67,8 @@ class LogRouter : public Logger { const std::string & message) noexcept override; private: - std::vector> loggers; + class Impl; + ImplPtr p_impl; }; } // namespace libdnf5 diff --git a/libdnf5/logger/log_router.cpp b/libdnf5/logger/log_router.cpp index ea57cee89..10ea17a6f 100644 --- a/libdnf5/logger/log_router.cpp +++ b/libdnf5/logger/log_router.cpp @@ -21,16 +21,48 @@ along with libdnf. If not, see . namespace libdnf5 { +class LogRouter::Impl { +public: + Impl(){}; + Impl(std::vector> && loggers) : loggers(std::move(loggers)) {} + +private: + friend LogRouter; + std::vector> loggers; +}; + +LogRouter::LogRouter() : p_impl(new Impl()){}; + +LogRouter::~LogRouter() = default; + +LogRouter::LogRouter(std::vector> && loggers) : p_impl(new Impl(std::move(loggers))) {} + +void LogRouter::add_logger(std::unique_ptr && logger) { + p_impl->loggers.push_back(std::move(logger)); +} + +Logger * LogRouter::get_logger(size_t index) { + return p_impl->loggers.at(index).get(); +} + +void LogRouter::swap_logger(std::unique_ptr & logger, size_t index) { + p_impl->loggers.at(index).swap(logger); +} + +size_t LogRouter::get_loggers_count() const noexcept { + return p_impl->loggers.size(); +} + std::unique_ptr LogRouter::release_logger(size_t index) { - auto ret = std::move(loggers.at(index)); - loggers.erase(loggers.begin() + static_cast(index)); + auto ret = std::move(p_impl->loggers.at(index)); + p_impl->loggers.erase(p_impl->loggers.begin() + static_cast(index)); return ret; } void LogRouter::log_line(Level level, const std::string & message) noexcept { auto now = std::chrono::system_clock::now(); auto pid = getpid(); - for (auto & logger : loggers) { + for (auto & logger : p_impl->loggers) { logger->write(now, pid, level, message); } } @@ -41,7 +73,7 @@ void LogRouter::write( pid_t pid, Level level, const std::string & message) noexcept { - for (auto & logger : loggers) { + for (auto & logger : p_impl->loggers) { logger->write(time, pid, level, message); } }