From 457d1b4490dbc293246f372532a81a9e90d366c4 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Fri, 20 Jul 2018 15:27:17 -0400 Subject: [PATCH 1/3] logging/backend: Use std::string_view in RemoveBackend() and GetBackend() These can just use a view to a string since its only comparing against two names in both cases for matches. This avoids constructing std::string instances where they aren't necessary. --- src/common/logging/backend.cpp | 20 ++++++++++---------- src/common/logging/backend.h | 5 +++-- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/common/logging/backend.cpp b/src/common/logging/backend.cpp index ed1e93cc2c..3745af9dfb 100644 --- a/src/common/logging/backend.cpp +++ b/src/common/logging/backend.cpp @@ -48,11 +48,11 @@ public: backends.push_back(std::move(backend)); } - void RemoveBackend(const std::string& backend_name) { + void RemoveBackend(std::string_view backend_name) { std::lock_guard lock(writing_mutex); - auto it = std::remove_if(backends.begin(), backends.end(), [&backend_name](const auto& i) { - return !strcmp(i->GetName(), backend_name.c_str()); - }); + const auto it = + std::remove_if(backends.begin(), backends.end(), + [&backend_name](const auto& i) { return backend_name == i->GetName(); }); backends.erase(it, backends.end()); } @@ -64,10 +64,10 @@ public: filter = f; } - Backend* GetBackend(const std::string& backend_name) { - auto it = std::find_if(backends.begin(), backends.end(), [&backend_name](const auto& i) { - return !strcmp(i->GetName(), backend_name.c_str()); - }); + Backend* GetBackend(std::string_view backend_name) { + const auto it = + std::find_if(backends.begin(), backends.end(), + [&backend_name](const auto& i) { return backend_name == i->GetName(); }); if (it == backends.end()) return nullptr; return it->get(); @@ -265,11 +265,11 @@ void AddBackend(std::unique_ptr backend) { Impl::Instance().AddBackend(std::move(backend)); } -void RemoveBackend(const std::string& backend_name) { +void RemoveBackend(std::string_view backend_name) { Impl::Instance().RemoveBackend(backend_name); } -Backend* GetBackend(const std::string& backend_name) { +Backend* GetBackend(std::string_view backend_name) { return Impl::Instance().GetBackend(backend_name); } diff --git a/src/common/logging/backend.h b/src/common/logging/backend.h index 57cdf6b2de..45609a535c 100644 --- a/src/common/logging/backend.h +++ b/src/common/logging/backend.h @@ -7,6 +7,7 @@ #include #include #include +#include #include #include "common/file_util.h" #include "common/logging/filter.h" @@ -106,9 +107,9 @@ private: void AddBackend(std::unique_ptr backend); -void RemoveBackend(const std::string& backend_name); +void RemoveBackend(std::string_view backend_name); -Backend* GetBackend(const std::string& backend_name); +Backend* GetBackend(std::string_view backend_name); /** * Returns the name of the passed log class as a C-string. Subclasses are separated by periods From 7a1a860abe6a6032353209a3df3841ee574877a8 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Fri, 20 Jul 2018 15:31:25 -0400 Subject: [PATCH 2/3] logging/backend: Add missing standard includes A few inclusions were being satisfied indirectly. To prevent breakages in the future, include these directly. --- src/common/logging/backend.cpp | 5 +++-- src/common/logging/backend.h | 2 -- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/common/logging/backend.cpp b/src/common/logging/backend.cpp index 3745af9dfb..59b999935b 100644 --- a/src/common/logging/backend.cpp +++ b/src/common/logging/backend.cpp @@ -3,19 +3,20 @@ // Refer to the license.txt file included. #include -#include +#include #include #include #include #include +#include #include +#include #ifdef _WIN32 #include // For _SH_DENYWR #else #define _SH_DENYWR 0 #endif #include "common/assert.h" -#include "common/common_funcs.h" // snprintf compatibility define #include "common/logging/backend.h" #include "common/logging/log.h" #include "common/logging/text_formatter.h" diff --git a/src/common/logging/backend.h b/src/common/logging/backend.h index 45609a535c..b3f4b9cef2 100644 --- a/src/common/logging/backend.h +++ b/src/common/logging/backend.h @@ -4,11 +4,9 @@ #pragma once #include -#include #include #include #include -#include #include "common/file_util.h" #include "common/logging/filter.h" #include "common/logging/log.h" From f63ccbd93640bfd1c8bb8aa68f1ed1b0e0a91e58 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Fri, 20 Jul 2018 15:42:42 -0400 Subject: [PATCH 3/3] logging/filter: Use std::string_view in ParseFilterString() Allows avoiding constructing std::string instances, since this only reads an arbitrary sequence of characters. We can also make ParseFilterRule() internal, since it doesn't depend on any private instance state of Filter --- src/common/logging/filter.cpp | 75 ++++++++++++++++++----------------- src/common/logging/filter.h | 6 +-- 2 files changed, 40 insertions(+), 41 deletions(-) diff --git a/src/common/logging/filter.cpp b/src/common/logging/filter.cpp index 6ed087bebe..2dd331152e 100644 --- a/src/common/logging/filter.cpp +++ b/src/common/logging/filter.cpp @@ -8,39 +8,9 @@ #include "common/string_util.h" namespace Log { - -Filter::Filter(Level default_level) { - ResetAll(default_level); -} - -void Filter::ResetAll(Level level) { - class_levels.fill(level); -} - -void Filter::SetClassLevel(Class log_class, Level level) { - class_levels[static_cast(log_class)] = level; -} - -void Filter::ParseFilterString(const std::string& filter_str) { - auto clause_begin = filter_str.cbegin(); - while (clause_begin != filter_str.cend()) { - auto clause_end = std::find(clause_begin, filter_str.cend(), ' '); - - // If clause isn't empty - if (clause_end != clause_begin) { - ParseFilterRule(clause_begin, clause_end); - } - - if (clause_end != filter_str.cend()) { - // Skip over the whitespace - ++clause_end; - } - clause_begin = clause_end; - } -} - +namespace { template -static Level GetLevelByName(const It begin, const It end) { +Level GetLevelByName(const It begin, const It end) { for (u8 i = 0; i < static_cast(Level::Count); ++i) { const char* level_name = GetLevelName(static_cast(i)); if (Common::ComparePartialString(begin, end, level_name)) { @@ -51,7 +21,7 @@ static Level GetLevelByName(const It begin, const It end) { } template -static Class GetClassByName(const It begin, const It end) { +Class GetClassByName(const It begin, const It end) { for (ClassType i = 0; i < static_cast(Class::Count); ++i) { const char* level_name = GetLogClassName(static_cast(i)); if (Common::ComparePartialString(begin, end, level_name)) { @@ -61,8 +31,8 @@ static Class GetClassByName(const It begin, const It end) { return Class::Count; } -bool Filter::ParseFilterRule(const std::string::const_iterator begin, - const std::string::const_iterator end) { +template +bool ParseFilterRule(Filter& instance, Iterator begin, Iterator end) { auto level_separator = std::find(begin, end, ':'); if (level_separator == end) { LOG_ERROR(Log, "Invalid log filter. Must specify a log level after `:`: {}", @@ -77,7 +47,7 @@ bool Filter::ParseFilterRule(const std::string::const_iterator begin, } if (Common::ComparePartialString(begin, level_separator, "*")) { - ResetAll(level); + instance.ResetAll(level); return true; } @@ -87,9 +57,40 @@ bool Filter::ParseFilterRule(const std::string::const_iterator begin, return false; } - SetClassLevel(log_class, level); + instance.SetClassLevel(log_class, level); return true; } +} // Anonymous namespace + +Filter::Filter(Level default_level) { + ResetAll(default_level); +} + +void Filter::ResetAll(Level level) { + class_levels.fill(level); +} + +void Filter::SetClassLevel(Class log_class, Level level) { + class_levels[static_cast(log_class)] = level; +} + +void Filter::ParseFilterString(std::string_view filter_view) { + auto clause_begin = filter_view.cbegin(); + while (clause_begin != filter_view.cend()) { + auto clause_end = std::find(clause_begin, filter_view.cend(), ' '); + + // If clause isn't empty + if (clause_end != clause_begin) { + ParseFilterRule(*this, clause_begin, clause_end); + } + + if (clause_end != filter_view.cend()) { + // Skip over the whitespace + ++clause_end; + } + clause_begin = clause_end; + } +} bool Filter::CheckMessage(Class log_class, Level level) const { return static_cast(level) >= static_cast(class_levels[static_cast(log_class)]); diff --git a/src/common/logging/filter.h b/src/common/logging/filter.h index 2a4f7c8452..d5ffc5a581 100644 --- a/src/common/logging/filter.h +++ b/src/common/logging/filter.h @@ -6,7 +6,7 @@ #include #include -#include +#include #include "common/logging/log.h" namespace Log { @@ -40,9 +40,7 @@ public: * - `Service:Info` -- Sets the level of Service to Info. * - `Service.FS:Trace` -- Sets the level of the Service.FS class to Trace. */ - void ParseFilterString(const std::string& filter_str); - bool ParseFilterRule(const std::string::const_iterator start, - const std::string::const_iterator end); + void ParseFilterString(std::string_view filter_view); /// Matches class/level combination against the filter, returning true if it passed. bool CheckMessage(Class log_class, Level level) const;