From 7b78425d6b42a7ca6ddf39cf2f43fcbdc01d85a3 Mon Sep 17 00:00:00 2001 From: James Rowe Date: Mon, 19 Feb 2018 21:35:57 -0700 Subject: [PATCH] Address review comments --- src/common/logging/backend.cpp | 30 +++++++++++++-------------- src/common/logging/backend.h | 7 ++++--- src/common/logging/text_formatter.cpp | 5 ++++- 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/common/logging/backend.cpp b/src/common/logging/backend.cpp index 133e63ed0..dbc11fb1e 100644 --- a/src/common/logging/backend.cpp +++ b/src/common/logging/backend.cpp @@ -32,8 +32,8 @@ public: Impl(Impl const&) = delete; const Impl& operator=(Impl const&) = delete; - Common::MPSCQueue& GetQueue() { - return message_queue; + void PushEntry(Entry e) { + message_queue.Push(std::move(e)); } void AddBackend(std::unique_ptr backend) { @@ -53,17 +53,17 @@ public: filter = f; } - Backend* GetBackend(const std::string& backend_name) { + boost::optional GetBackend(const std::string& backend_name) { auto it = std::find_if(backends.begin(), backends.end(), [&backend_name](const auto& i) { return i->GetName() == backend_name; }); if (it == backends.end()) - return nullptr; + return boost::none; return it->get(); } private: - Impl() : running(true) { + Impl() { backend_thread = std::async(std::launch::async, [&] { using namespace std::chrono_literals; Entry entry; @@ -79,12 +79,10 @@ private: }); } ~Impl() { - if (running) { - running = false; - } + running = false; } - std::atomic_bool running; + std::atomic_bool running{true}; std::future backend_thread; std::vector> backends; Common::MPSCQueue message_queue; @@ -103,7 +101,7 @@ void FileBackend::Write(const Entry& entry) { if (!file.IsOpen()) { return; } - file.WriteString(FormatLogMessage(entry) + "\n"); + file.WriteString(FormatLogMessage(entry) + '\n'); } /// Macro listing all log classes. Code should define CLS and SUB as desired before invoking this. @@ -205,7 +203,7 @@ const char* GetLevelName(Level log_level) { } Entry CreateEntry(Class log_class, Level log_level, const char* filename, unsigned int line_nr, - const char* function, const std::string& message) { + const char* function, std::string message) { using std::chrono::duration_cast; using std::chrono::steady_clock; @@ -215,9 +213,9 @@ Entry CreateEntry(Class log_class, Level log_level, const char* filename, unsign entry.timestamp = duration_cast(steady_clock::now() - time_origin); entry.log_class = log_class; entry.log_level = log_level; - entry.filename = std::string(Common::TrimSourcePath(filename)); + entry.filename = Common::TrimSourcePath(filename); entry.line_num = line_nr; - entry.function = std::string(function); + entry.function = function; entry.message = message; return entry; @@ -235,7 +233,7 @@ void RemoveBackend(const std::string& backend_name) { Impl::Instance().RemoveBackend(backend_name); } -Backend* GetBackend(const std::string& backend_name) { +boost::optional GetBackend(const std::string& backend_name) { return Impl::Instance().GetBackend(backend_name); } @@ -252,7 +250,7 @@ void LogMessage(Class log_class, Level log_level, const char* filename, unsigned Entry entry = CreateEntry(log_class, log_level, filename, line_num, function, std::string(formatting_buffer.data())); - Impl::Instance().GetQueue().Push(std::move(entry)); + Impl::Instance().PushEntry(std::move(entry)); } void FmtLogMessage(Class log_class, Level log_level, const char* filename, unsigned int line_num, @@ -264,6 +262,6 @@ void FmtLogMessage(Class log_class, Level log_level, const char* filename, unsig Entry entry = CreateEntry(log_class, log_level, filename, line_num, function, fmt::format(format, args)); - Impl::Instance().GetQueue().Push(std::move(entry)); + Impl::Instance().PushEntry(std::move(entry)); } } // namespace Log diff --git a/src/common/logging/backend.h b/src/common/logging/backend.h index 93d982c9d..dfa4944f4 100644 --- a/src/common/logging/backend.h +++ b/src/common/logging/backend.h @@ -9,6 +9,7 @@ #include #include #include +#include #include "common/file_util.h" #include "common/logging/filter.h" #include "common/logging/log.h" @@ -81,7 +82,7 @@ public: */ class FileBackend : public Backend { public: - FileBackend(const std::string& filename) : file(filename, "w") {} + explicit FileBackend(const std::string& filename) : file(filename, "w") {} const char* GetName() const override { return "file"; @@ -97,7 +98,7 @@ void AddBackend(std::unique_ptr backend); void RemoveBackend(const std::string& backend_name); -Backend* GetBackend(const std::string& backend_name); +boost::optional GetBackend(const std::string& backend_name); /** * Returns the name of the passed log class as a C-string. Subclasses are separated by periods @@ -112,7 +113,7 @@ const char* GetLevelName(Level log_level); /// Creates a log entry by formatting the given source location, and message. Entry CreateEntry(Class log_class, Level log_level, const char* filename, unsigned int line_nr, - const char* function, const char* format, const std::string& message); + const char* function, std::string message); /** * The global filter will prevent any messages from even being processed if they are filtered. Each diff --git a/src/common/logging/text_formatter.cpp b/src/common/logging/text_formatter.cpp index 8382ad5da..8583916a8 100644 --- a/src/common/logging/text_formatter.cpp +++ b/src/common/logging/text_formatter.cpp @@ -31,13 +31,16 @@ std::string FormatLogMessage(const Entry& entry) { } void PrintMessage(const Entry& entry) { - auto str = FormatLogMessage(entry) + "\n"; + auto str = FormatLogMessage(entry) + '\n'; fputs(str.c_str(), stderr); } void PrintColoredMessage(const Entry& entry) { #ifdef _WIN32 HANDLE console_handle = GetStdHandle(STD_ERROR_HANDLE); + if (console_handle == INVALID_HANDLE_VALUE) { + return; + } CONSOLE_SCREEN_BUFFER_INFO original_info = {0}; GetConsoleScreenBufferInfo(console_handle, &original_info);