From 8e2cf08be1b9e8a89ea9815562194d4b53e33327 Mon Sep 17 00:00:00 2001 From: Alexander Shaduri Date: Wed, 3 Mar 2021 17:40:21 +0400 Subject: [PATCH] Cleaned up libdebug for clang-tidy warnings. --- .clang-tidy | 52 +++++++++++++++++----- src/libdebug/dchannel.cpp | 12 ++--- src/libdebug/dcmdarg.cpp | 21 +++++---- src/libdebug/dflags.h | 16 ++++--- src/libdebug/dout.cpp | 18 ++++---- src/libdebug/dstate.cpp | 49 ++++++++++---------- src/libdebug/dstate.h | 4 +- src/libdebug/dstream.cpp | 4 +- src/libdebug/examples/example_libdebug.cpp | 5 ++- 9 files changed, 108 insertions(+), 73 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 0dc85d7..481c057 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,9 +1,23 @@ --- # Exceptions: +# bugprone-branch-clone is too noisy in parsing code. +# cert-dcl50-cpp is triggered by debug_print()'s C style. +# clang-analyzer-deadcode.DeadStores is too noisy. +# clang-analyzer-cplusplus.NewDeleteLeaks doesn't understand Gtkmm memory management. +# cppcoreguidelines-avoid-magic-numbers, readability-magic-numbers is triggered by attribute database. +# cppcoreguidelines-owning-memory doesn't support deleting Gtkmm objects. # cppcoreguidelines-pro-type-cstyle-cast needed by GTK C casts. -# readability-function-cognitive-complexity needed by UI constructors. +# cppcoreguidelines-pro-type-vararg is triggered by libdebug's debug_print(). +# cppcoreguidelines-pro-bounds-array-to-pointer-decay is triggered by va_start in libdebug's debug_print(). +# cppcoreguidelines-pro-bounds-pointer-arithmetic is useless (triggered by argv) until we have std::span. +# misc-no-recursion is too noisy. +# modernize-raw-string-literal is very noisy. +# modernize-use-trailing-return-type is contrary to our style. # readability-convert-member-functions-to-static is triggered for many callbacks. +# readability-function-cognitive-complexity needed by UI constructors. +# readability-isolate-declaration is noisy. + Checks: > -abseil-*, @@ -11,11 +25,20 @@ Checks: > -android-*, -boost-*, bugprone-*, + -bugprone-branch-clone, cert-*, + -cert-dcl50-cpp, clang-analyzer-*, + -clang-analyzer-deadcode.DeadStores, + -clang-analyzer-cplusplus.NewDeleteLeaks, concurrency-*, cppcoreguidelines-*, + -cppcoreguidelines-avoid-magic-numbers, + -cppcoreguidelines-owning-memory, + -cppcoreguidelines-pro-bounds-array-to-pointer-decay, -cppcoreguidelines-pro-type-cstyle-cast, + -cppcoreguidelines-pro-type-vararg, + -cppcoreguidelines-pro-bounds-pointer-arithmetic, -darwin-*, -fuchsia-*, google-* @@ -24,8 +47,10 @@ Checks: > -llvm-*, -llvmlibc-*, misc-*, + -misc-no-recursion, modernize-*, -modernize-use-trailing-return-type, + -modernize-raw-string-literal, -mpi-*, -objc-*, openmp-*, @@ -34,28 +59,35 @@ Checks: > readability-*, -readability-convert-member-functions-to-static, -readability-function-cognitive-complexity, + -readability-isolate-declaration, + -readability-magic-numbers, -zircon-* WarningsAsErrors: '' HeaderFilterRegex: '' FormatStyle: none -# Fixits should align pointer to left -DerivePointerAlignment: false -PointerAlignment: Left - CheckOptions: - - key: readability-braces-around-statements.ShortStatementLines - value: '2' + - key: bugprone-exception-escape.CheckCapsOnly + value: '1' + - key: cppcoreguidelines-macro-usage.CheckCapsOnly + value: '1' + - key: cppcoreguidelines-owning-memory.LegacyResourceConsumers + value: '::free;::realloc;::freopen;::fclose;Gtk::manage' - key: misc-assert-side-effect.AssertMacros value: assert,DBG_ASSERT - key: misc-assert-side-effect.CheckFunctionCalls value: '0' - - key: readability-implicit-bool-conversion.AllowConditionalPointerCasts + - key: misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic + value: '1' + - key: performance-unnecessary-value-param.AllowedTypes + value: 'RefPtr;Ptr$' + - key: readability-braces-around-statements.ShortStatementLines + value: '2' + - key: readability-implicit-bool-conversion.AllowPointerConditions value: '1' - key: readability-implicit-bool-cast.AllowConditionalPointerCasts value: '1' - - key: performance-unnecessary-value-param.AllowedTypes - value: 'RefPtr;Ptr$' + ... diff --git a/src/libdebug/dchannel.cpp b/src/libdebug/dchannel.cpp index 3b36465..147f54e 100644 --- a/src/libdebug/dchannel.cpp +++ b/src/libdebug/dchannel.cpp @@ -26,14 +26,14 @@ std::string debug_format_message(debug_level::flag level, const std::string& dom ret.reserve(msg.size() + 40); // indentation + domain/level // allow prefix only for first line and others when first_line_only is disabled - if (is_first_line || !(format_flags.to_ulong() & debug_format::first_line_only)) { + if (is_first_line || !format_flags.test(debug_format::first_line_only)) { - if (format_flags.to_ulong() & debug_format::datetime) { // print time + if (format_flags.test(debug_format::datetime)) { // print time ret += hz::format_date("%Y-%m-%d %H:%M:%S: ", true); } - if (format_flags.to_ulong() & debug_format::level) { // print level name - bool use_color = bool(format_flags.to_ulong() & debug_format::color); + if (format_flags.test(debug_format::level)) { // print level name + bool use_color = format_flags.test(debug_format::color); if (use_color) ret += debug_level::get_color_start(level); @@ -44,14 +44,14 @@ std::string debug_format_message(debug_level::flag level, const std::string& dom ret += debug_level::get_color_stop(level); } - if (format_flags.to_ulong() & debug_format::domain) { // print domain name + if (format_flags.test(debug_format::domain)) { // print domain name ret += std::string("[") + domain + "] "; } } - if (format_flags.to_ulong() & debug_format::indent) { + if (format_flags.test(debug_format::indent)) { std::string spaces(static_cast(indent_level * 4), ' '); // indentation spaces // replace all newlines with \n(indent-spaces) except for the last one. diff --git a/src/libdebug/dcmdarg.cpp b/src/libdebug/dcmdarg.cpp index 5321e24..3d5456a 100644 --- a/src/libdebug/dcmdarg.cpp +++ b/src/libdebug/dcmdarg.cpp @@ -89,12 +89,12 @@ static gboolean debug_internal_parse_levels([[maybe_unused]] const gchar* option const gchar* value, gpointer data, [[maybe_unused]] GError** error) { if (!value) - return false; + return FALSE; auto* args = static_cast(data); std::string levels = value; hz::string_split(levels, ',', args->debug_levels, true); // will filter out invalid ones later - return true; + return TRUE; } @@ -118,10 +118,10 @@ static gboolean debug_internal_post_parse_func([[maybe_unused]] GOptionContext* args->levels_enabled |= (std::find(args->debug_levels.begin(), args->debug_levels.end(), "fatal") != args->debug_levels.end() ? debug_level::fatal : debug_level::none); - } else if (args->quiet) { + } else if (args->quiet == TRUE) { args->levels_enabled = debug_level::none; - } else if (args->verbose) { + } else if (args->verbose == TRUE) { args->levels_enabled = debug_level::all; } else { @@ -138,7 +138,7 @@ static gboolean debug_internal_post_parse_func([[maybe_unused]] GOptionContext* unsigned long levels_enabled_ulong = args->levels_enabled.to_ulong(); - debug_internal::DebugState::domain_map_t& dm = debug_internal::get_debug_state().get_domain_map(); + debug_internal::DebugState::domain_map_t& dm = debug_internal::get_debug_state_ref().get_domain_map_ref(); for (auto& iter : dm) { for (auto& iter2 : iter.second) { @@ -154,7 +154,7 @@ static gboolean debug_internal_post_parse_func([[maybe_unused]] GOptionContext* } } - return true; + return TRUE; } @@ -163,7 +163,7 @@ static gboolean debug_internal_post_parse_func([[maybe_unused]] GOptionContext* std::string debug_get_cmd_args_dump() { - debug_internal::DebugCmdArgs* args = debug_internal::debug_get_args_holder(); + debug_internal::DebugCmdArgs* args = debug_internal::get_debug_get_args_holder(); std::ostringstream ss; // ss << "\tverbose: " << std::boolalpha << static_cast(args->verbose) << "\n"; @@ -183,14 +183,13 @@ std::string debug_get_cmd_args_dump() // no need to free the result. GOptionGroup* debug_get_option_group() { - debug_internal::DebugCmdArgs* args = debug_internal::debug_get_args_holder(); + debug_internal::DebugCmdArgs* args = debug_internal::get_debug_get_args_holder(); GOptionGroup* group = g_option_group_new("debug", "Libdebug Logging Options", "Show libdebug options", args, nullptr); - static const GOptionEntry entries[] = - { + static const std::vector entries = { { "verbose", 'v', G_OPTION_FLAG_IN_MAIN, G_OPTION_ARG_NONE, &(args->verbose), "Enable verbose logging; same as --verbosity-level 5", nullptr }, { "quiet", 'q', G_OPTION_FLAG_IN_MAIN, G_OPTION_ARG_NONE, @@ -207,7 +206,7 @@ GOptionGroup* debug_get_option_group() { nullptr, '\0', 0, G_OPTION_ARG_NONE, nullptr, nullptr, nullptr } }; - g_option_group_add_entries(group, entries); + g_option_group_add_entries(group, entries.data()); g_option_group_set_parse_hooks(group, nullptr, &debug_internal_post_parse_func); diff --git a/src/libdebug/dflags.h b/src/libdebug/dflags.h index d10d89e..191ad2b 100644 --- a/src/libdebug/dflags.h +++ b/src/libdebug/dflags.h @@ -44,12 +44,16 @@ namespace debug_level { template inline void get_matched_levels_array(const types& levels, Container& put_here) { - unsigned long levels_ulong = levels.to_ulong(); - if (levels_ulong & debug_level::dump) put_here.push_back(debug_level::dump); - if (levels_ulong & debug_level::info) put_here.push_back(debug_level::info); - if (levels_ulong & debug_level::warn) put_here.push_back(debug_level::warn); - if (levels_ulong & debug_level::error) put_here.push_back(debug_level::error); - if (levels_ulong & debug_level::fatal) put_here.push_back(debug_level::fatal); + for (auto level : { + debug_level::dump, + debug_level::info, + debug_level::warn, + debug_level::error, + debug_level::fatal }) { + if (levels.test(level)) { + put_here.push_back(level); + } + } } } diff --git a/src/libdebug/dout.cpp b/src/libdebug/dout.cpp index 549923e..dc57bc7 100644 --- a/src/libdebug/dout.cpp +++ b/src/libdebug/dout.cpp @@ -27,7 +27,7 @@ Copyright: // This may throw for invalid domain or level. std::ostream& debug_out(debug_level::flag level, const std::string& domain) { - auto& dm = debug_internal::get_debug_state().get_domain_map(); + auto& dm = debug_internal::get_debug_state_ref().get_domain_map_ref(); auto level_map = dm.find(domain); if (level_map == dm.end()) { // no such domain @@ -71,15 +71,15 @@ void debug_print(debug_level::flag level, const std::string& domain, const char* void debug_begin() { - debug_internal::get_debug_state().push_inside_begin(); + debug_internal::get_debug_state_ref().push_inside_begin(); } void debug_end() { - debug_internal::get_debug_state().pop_inside_begin(); + debug_internal::get_debug_state_ref().pop_inside_begin(); // this is needed because else the contents won't be written until next write. - debug_internal::get_debug_state().force_output(); + debug_internal::get_debug_state_ref().force_output(); } @@ -129,24 +129,24 @@ namespace debug_internal { // increase indentation level for all debug levels void debug_indent_inc(int by) { - int curr = debug_internal::get_debug_state().get_indent_level(); - debug_internal::get_debug_state().set_indent_level(curr + by); + int curr = debug_internal::get_debug_state_ref().get_indent_level(); + debug_internal::get_debug_state_ref().set_indent_level(curr + by); } void debug_indent_dec(int by) { - int curr = debug_internal::get_debug_state().get_indent_level(); + int curr = debug_internal::get_debug_state_ref().get_indent_level(); curr -= by; if (curr < 0) curr = 0; - debug_internal::get_debug_state().set_indent_level(curr); + debug_internal::get_debug_state_ref().set_indent_level(curr); } void debug_indent_reset() { - debug_internal::get_debug_state().set_indent_level(0); + debug_internal::get_debug_state_ref().set_indent_level(0); } diff --git a/src/libdebug/dstate.cpp b/src/libdebug/dstate.cpp index 64bb240..8364a94 100644 --- a/src/libdebug/dstate.cpp +++ b/src/libdebug/dstate.cpp @@ -37,29 +37,28 @@ namespace debug_internal { format_flags |= debug_format::color; #endif - std::map levels; - unsigned long levels_enabled_ulong = levels_enabled.to_ulong(); - levels[debug_level::dump] = bool(levels_enabled_ulong & debug_level::dump); - levels[debug_level::info] = bool(levels_enabled_ulong & debug_level::info); - levels[debug_level::warn] = bool(levels_enabled_ulong & debug_level::warn); - levels[debug_level::error] = bool(levels_enabled_ulong & debug_level::error); - levels[debug_level::fatal] = bool(levels_enabled_ulong & debug_level::fatal); + std::map levels = { + {debug_level::dump, levels_enabled.test(debug_level::dump) }, + {debug_level::info, levels_enabled.test(debug_level::info) }, + {debug_level::warn, levels_enabled.test(debug_level::warn) }, + {debug_level::error, levels_enabled.test(debug_level::error) }, + {debug_level::fatal, levels_enabled.test(debug_level::fatal) }, + }; - domain_map_t& dm = get_domain_map(); + domain_map_t& dm = get_domain_map_ref(); - dm["default"] = level_map_t(); + dm["default"] = {}; level_map_t& level_map = dm.find("default")->second; // we add the same copy to save memory and to ensure proper std::cerr locking. auto channel = std::make_shared(std::cerr); - for (auto& iter : levels) { - debug_level::flag level = iter.first; + for (const auto& [level, enabled] : levels) { level_map[level] = std::make_shared(level, "default", format_flags); level_map[level]->add_channel(channel); // add by smartpointer - level_map[level]->set_enabled(iter.second); + level_map[level]->set_enabled(enabled); } } @@ -67,12 +66,10 @@ namespace debug_internal { /// Global libdebug state. /// This will initialize the default domain and channels automatically. - static DebugState s_debug_state; - - - DebugState& get_debug_state() + DebugState& get_debug_state_ref() { - return s_debug_state; + static DebugState state; + return state; } @@ -86,7 +83,7 @@ bool debug_register_domain(const std::string& domain) { using namespace debug_internal; - DebugState::domain_map_t& dm = get_debug_state().get_domain_map(); + DebugState::domain_map_t& dm = get_debug_state_ref().get_domain_map_ref(); if (dm.find(domain) != dm.end()) // already exists return false; @@ -115,7 +112,7 @@ bool debug_register_domain(const std::string& domain) bool debug_unregister_domain(const std::string& domain) { using namespace debug_internal; - DebugState::domain_map_t& dm = get_debug_state().get_domain_map(); + DebugState::domain_map_t& dm = get_debug_state_ref().get_domain_map_ref(); auto found = dm.find(domain); if (found == dm.end()) // doesn't exists @@ -130,7 +127,7 @@ bool debug_unregister_domain(const std::string& domain) std::vector debug_get_registered_domains() { using namespace debug_internal; - DebugState::domain_map_t& dm = get_debug_state().get_domain_map(); + DebugState::domain_map_t& dm = get_debug_state_ref().get_domain_map_ref(); std::vector domains; domains.reserve(dm.size()); @@ -147,7 +144,7 @@ std::vector debug_get_registered_domains() bool debug_set_enabled(const std::string& domain, const debug_level::types& levels, bool enabled) { using namespace debug_internal; - DebugState::domain_map_t& dm = get_debug_state().get_domain_map(); + DebugState::domain_map_t& dm = get_debug_state_ref().get_domain_map_ref(); if (domain == "all") { bool status = true; @@ -174,7 +171,7 @@ bool debug_set_enabled(const std::string& domain, const debug_level::types& leve debug_level::types debug_get_enabled(const std::string& domain) { using namespace debug_internal; - DebugState::domain_map_t& dm = get_debug_state().get_domain_map(); + DebugState::domain_map_t& dm = get_debug_state_ref().get_domain_map_ref(); debug_level::types levels; @@ -198,7 +195,7 @@ debug_level::types debug_get_enabled(const std::string& domain) bool debug_set_format(const std::string& domain, const debug_level::types& levels, const debug_format::type& format) { using namespace debug_internal; - DebugState::domain_map_t& dm = get_debug_state().get_domain_map(); + DebugState::domain_map_t& dm = get_debug_state_ref().get_domain_map_ref(); if (domain == "all") { bool status = true; @@ -225,7 +222,7 @@ bool debug_set_format(const std::string& domain, const debug_level::types& level std::map debug_get_formats(const std::string& domain) { using namespace debug_internal; - DebugState::domain_map_t& dm = get_debug_state().get_domain_map(); + DebugState::domain_map_t& dm = get_debug_state_ref().get_domain_map_ref(); std::map formats; @@ -245,7 +242,7 @@ std::map debug_get_formats(const std::str bool debug_add_channel(const std::string& domain, const debug_level::types& levels, const DebugChannelBasePtr& channel) { using namespace debug_internal; - DebugState::domain_map_t& dm = get_debug_state().get_domain_map(); + DebugState::domain_map_t& dm = get_debug_state_ref().get_domain_map_ref(); if (domain == "all") { bool status = true; @@ -273,7 +270,7 @@ bool debug_add_channel(const std::string& domain, const debug_level::types& leve bool debug_clear_channels(const std::string& domain, const debug_level::types& levels) { using namespace debug_internal; - DebugState::domain_map_t& dm = get_debug_state().get_domain_map(); + DebugState::domain_map_t& dm = get_debug_state_ref().get_domain_map_ref(); if (domain == "all") { bool status = true; diff --git a/src/libdebug/dstate.h b/src/libdebug/dstate.h index 0f39990..a2a628e 100644 --- a/src/libdebug/dstate.h +++ b/src/libdebug/dstate.h @@ -56,7 +56,7 @@ namespace debug_internal { /// Get the domain/level mapping. - [[nodiscard]] domain_map_t& get_domain_map() + [[nodiscard]] domain_map_t& get_domain_map_ref() { return domain_map; } @@ -121,7 +121,7 @@ namespace debug_internal { /// Get global libdebug state - DebugState& get_debug_state(); + DebugState& get_debug_state_ref(); diff --git a/src/libdebug/dstream.cpp b/src/libdebug/dstream.cpp index 5261e0d..bc67dae 100644 --- a/src/libdebug/dstream.cpp +++ b/src/libdebug/dstream.cpp @@ -51,7 +51,7 @@ namespace debug_internal { { debug_format::type flags = dos_->format_; bool is_first_line = false; - if (get_debug_state().get_inside_begin()) { + if (get_debug_state_ref().get_inside_begin()) { flags |= debug_format::first_line_only; if (dos_->get_is_first_line()) { dos_->set_is_first_line(false); // tls @@ -65,7 +65,7 @@ namespace debug_internal { for (auto & channel : dos_->channels_) { // send() locks the channel if needed channel->send(dos_->level_, dos_->domain_, flags, - get_debug_state().get_indent_level(), is_first_line, oss_.str()); + get_debug_state_ref().get_indent_level(), is_first_line, oss_.str()); } oss_.str(""); // clear the buffer oss_.clear(); // clear the flags diff --git a/src/libdebug/examples/example_libdebug.cpp b/src/libdebug/examples/example_libdebug.cpp index 72a2196..1944846 100644 --- a/src/libdebug/examples/example_libdebug.cpp +++ b/src/libdebug/examples/example_libdebug.cpp @@ -69,7 +69,10 @@ int main() debug_set_enabled("dom", debug_level::dump, false); debug_set_format("dom", debug_level::info, - (debug_get_formats("dom")[debug_level::info].to_ulong() & ~debug_format::color) | debug_format::datetime); + (debug_get_formats("dom")[debug_level::info] + .reset(debug_format::color) + .set(debug_format::datetime) + )); std::string something = "some thing";