From 3041faffab1bf0aba46cbcf51b697336d0e846fa Mon Sep 17 00:00:00 2001 From: Joe Burzinski Date: Tue, 2 Jun 2020 20:30:25 -0500 Subject: [PATCH] Address code review comments: revert perfect forwarding on places that didn't need it, remove negative compilation unit test. --- CMakeLists.txt | 1 - include/spdlog/logger.h | 94 ++++++++++++++--------------- include/spdlog/spdlog.h | 60 +++++++++--------- tests/CMakeLists.txt | 26 -------- tests/test_compilation_failures.cpp | 14 ----- tests/test_misc.cpp | 35 +---------- 6 files changed, 79 insertions(+), 151 deletions(-) delete mode 100644 tests/test_compilation_failures.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 7b792cab..f8941df3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -71,7 +71,6 @@ option(SPDLOG_BUILD_EXAMPLE_HO "Build header only example" OFF) # testing options option(SPDLOG_BUILD_TESTS "Build tests" OFF) option(SPDLOG_BUILD_TESTS_HO "Build tests using the header only version" OFF) -option(SPDLOG_BUILD_FAILING_TESTS "Add unit test targets that test for compilation failures" OFF) # bench options option(SPDLOG_BUILD_BENCH "Build benchmarks (Requires https://github.com/google/benchmark.git to be installed)" OFF) diff --git a/include/spdlog/logger.h b/include/spdlog/logger.h index ed44f526..6cf99e13 100644 --- a/include/spdlog/logger.h +++ b/include/spdlog/logger.h @@ -76,71 +76,71 @@ public: // FormatString is a type derived from fmt::compile_string template::value, int>::type = 0, typename... Args> - void log(source_loc loc, level::level_enum lvl, FormatString &&fmt, Args &&... args) + void log(source_loc loc, level::level_enum lvl, FormatString &&fmt, const Args &... args) { - log_(loc, lvl, std::forward(fmt), std::forward(args)...); + log_(loc, lvl, std::forward(fmt), args...); } // FormatString is NOT a type derived from fmt::compile_string but is a string_view_t or can be implicitly converted to one template - void log(source_loc loc, level::level_enum lvl, string_view_t fmt, Args &&... args) + void log(source_loc loc, level::level_enum lvl, string_view_t fmt, const Args &... args) { - log_(loc, lvl, fmt, std::forward(args)...); + log_(loc, lvl, fmt, args...); } template - void log(level::level_enum lvl, FormatString &&fmt, Args &&... args) + void log(level::level_enum lvl, FormatString &&fmt, const Args &... args) { - log(source_loc{}, lvl, std::forward(fmt), std::forward(args)...); + log(source_loc{}, lvl, std::forward(fmt), args...); } template - void trace(FormatString &&fmt, Args &&... args) + void trace(FormatString &&fmt, const Args &... args) { - log(level::trace, std::forward(fmt), std::forward(args)...); + log(level::trace, std::forward(fmt), args...); } template - void debug(FormatString &&fmt, Args &&... args) + void debug(FormatString &&fmt, const Args &... args) { - log(level::debug, std::forward(fmt), std::forward(args)...); + log(level::debug, std::forward(fmt), args...); } template - void info(FormatString &&fmt, Args &&... args) + void info(FormatString &&fmt, const Args &... args) { - log(level::info, std::forward(fmt), std::forward(args)...); + log(level::info, std::forward(fmt), args...); } template - void warn(FormatString &&fmt, Args &&... args) + void warn(FormatString &&fmt, const Args &... args) { - log(level::warn, std::forward(fmt), std::forward(args)...); + log(level::warn, std::forward(fmt), args...); } template - void error(FormatString &&fmt, Args &&... args) + void error(FormatString &&fmt, const Args &... args) { - log(level::err, std::forward(fmt), std::forward(args)...); + log(level::err, std::forward(fmt), args...); } template - void critical(FormatString &&fmt, Args &&... args) + void critical(FormatString &&fmt, const Args &... args) { - log(level::critical, std::forward(fmt), std::forward(args)...); + log(level::critical, std::forward(fmt), args...); } template - void log(level::level_enum lvl, T &&msg) + void log(level::level_enum lvl, const T &msg) { - log(source_loc{}, lvl, std::forward(msg)); + log(source_loc{}, lvl, msg); } // T can be statically converted to string_view - template::value, T>::type * = nullptr> - void log(source_loc loc, level::level_enum lvl, T &&msg) + template::value, T>::type * = nullptr> + void log(source_loc loc, level::level_enum lvl, const T &msg) { - log(loc, lvl, string_view_t{std::forward(msg)}); + log(loc, lvl, string_view_t{msg}); } void log(log_clock::time_point log_time, source_loc loc, level::level_enum lvl, string_view_t msg) @@ -175,48 +175,48 @@ public: } // T cannot be statically converted to string_view or wstring_view - template::value && !is_convertible_to_wstring_view::value, - T>::type * = nullptr> - void log(source_loc loc, level::level_enum lvl, T &&msg) + template::value && + !is_convertible_to_wstring_view::value, + T>::type * = nullptr> + void log(source_loc loc, level::level_enum lvl, const T &msg) { - log(loc, lvl, "{}", std::forward(msg)); + log(loc, lvl, "{}", msg); } template - void trace(T &&msg) + void trace(const T &msg) { - log(level::trace, std::forward(msg)); + log(level::trace, msg); } template - void debug(T &&msg) + void debug(const T &msg) { - log(level::debug, std::forward(msg)); + log(level::debug, msg); } template - void info(T &&msg) + void info(const T &msg) { - log(level::info, std::forward(msg)); + log(level::info, msg); } template - void warn(T &&msg) + void warn(const T &msg) { - log(level::warn, std::forward(msg)); + log(level::warn, msg); } template - void error(T &&msg) + void error(const T &msg) { - log(level::err, std::forward(msg)); + log(level::err, msg); } template - void critical(T &&msg) + void critical(const T &msg) { - log(level::critical, std::forward(msg)); + log(level::critical, msg); } #ifdef SPDLOG_WCHAR_TO_UTF8_SUPPORT @@ -225,7 +225,7 @@ public: #else template - void log(source_loc loc, level::level_enum lvl, wstring_view_t fmt, Args &&... args) + void log(source_loc loc, level::level_enum lvl, wstring_view_t fmt, const Args &... args) { bool log_enabled = should_log(lvl); bool traceback_enabled = tracer_.enabled(); @@ -237,7 +237,7 @@ public: { // format to wmemory_buffer and convert to utf8 fmt::wmemory_buffer wbuf; - fmt::format_to(wbuf, fmt, std::forward(args)...); + fmt::format_to(wbuf, fmt, args...); memory_buf_t buf; details::os::wstr_to_utf8buf(wstring_view_t(wbuf.data(), wbuf.size()), buf); @@ -248,8 +248,8 @@ public: } // T can be statically converted to wstring_view - template::value, T>::type * = nullptr> - void log(source_loc loc, level::level_enum lvl, T &&msg) + template::value, T>::type * = nullptr> + void log(source_loc loc, level::level_enum lvl, const T &msg) { bool log_enabled = should_log(lvl); bool traceback_enabled = tracer_.enabled(); @@ -261,7 +261,7 @@ public: SPDLOG_TRY { memory_buf_t buf; - details::os::wstr_to_utf8buf(std::forward(msg), buf); + details::os::wstr_to_utf8buf(msg, buf); details::log_msg log_msg(loc, name_, lvl, string_view_t(buf.data(), buf.size())); log_it_(log_msg, log_enabled, traceback_enabled); } @@ -326,7 +326,7 @@ protected: // common implementation for after templated public api has been resolved template - void log_(source_loc loc, level::level_enum lvl, FormatString &&fmt, Args &&... args) + void log_(source_loc loc, level::level_enum lvl, FormatString &&fmt, const Args &... args) { bool log_enabled = should_log(lvl); bool traceback_enabled = tracer_.enabled(); @@ -337,7 +337,7 @@ protected: SPDLOG_TRY { memory_buf_t buf; - fmt::format_to(buf, std::forward(fmt), std::forward(args)...); + fmt::format_to(buf, std::forward(fmt), args...); details::log_msg log_msg(loc, name_, lvl, string_view_t(buf.data(), buf.size())); log_it_(log_msg, log_enabled, traceback_enabled); } diff --git a/include/spdlog/spdlog.h b/include/spdlog/spdlog.h index 1407e7c0..44c0d0d9 100644 --- a/include/spdlog/spdlog.h +++ b/include/spdlog/spdlog.h @@ -122,45 +122,45 @@ SPDLOG_API spdlog::logger *default_logger_raw(); SPDLOG_API void set_default_logger(std::shared_ptr default_logger); template -inline void log(source_loc source, level::level_enum lvl, FormatString &&fmt, Args &&... args) +inline void log(source_loc source, level::level_enum lvl, FormatString &&fmt, const Args &... args) { - default_logger_raw()->log(source, lvl, std::forward(fmt), std::forward(args)...); + default_logger_raw()->log(source, lvl, std::forward(fmt), args...); } template -inline void log(level::level_enum lvl, FormatString &&fmt, Args &&... args) +inline void log(level::level_enum lvl, FormatString &&fmt, const Args &... args) { - default_logger_raw()->log(source_loc{}, lvl, std::forward(fmt), std::forward(args)...); + default_logger_raw()->log(source_loc{}, lvl, std::forward(fmt), args...); } template -inline void trace(FormatString &&fmt, Args &&... args) +inline void trace(FormatString &&fmt, const Args &... args) { - default_logger_raw()->trace(std::forward(fmt), std::forward(args)...); + default_logger_raw()->trace(std::forward(fmt), args...); } template -inline void debug(FormatString &&fmt, Args &&... args) +inline void debug(FormatString &&fmt, const Args &... args) { - default_logger_raw()->debug(std::forward(fmt), std::forward(args)...); + default_logger_raw()->debug(std::forward(fmt), args...); } template -inline void info(FormatString &&fmt, Args &&... args) +inline void info(FormatString &&fmt, const Args &... args) { - default_logger_raw()->info(std::forward(fmt), std::forward(args)...); + default_logger_raw()->info(std::forward(fmt), args...); } template -inline void warn(FormatString &&fmt, Args &&... args) +inline void warn(FormatString &&fmt, const Args &... args) { - default_logger_raw()->warn(std::forward(fmt), std::forward(args)...); + default_logger_raw()->warn(std::forward(fmt), args...); } template -inline void error(FormatString &&fmt, Args &&... args) +inline void error(FormatString &&fmt, const Args &... args) { - default_logger_raw()->error(std::forward(fmt), std::forward(args)...); + default_logger_raw()->error(std::forward(fmt), args...); } template @@ -170,51 +170,51 @@ inline void critical(FormatString &&fmt, const Args &... args) } template -inline void log(source_loc source, level::level_enum lvl, T &&msg) +inline void log(source_loc source, level::level_enum lvl, const T &msg) { - default_logger_raw()->log(source, lvl, std::forward(msg)); + default_logger_raw()->log(source, lvl, msg); } template -inline void log(level::level_enum lvl, T &&msg) +inline void log(level::level_enum lvl, const T &msg) { - default_logger_raw()->log(lvl, std::forward(msg)); + default_logger_raw()->log(lvl, msg); } template -inline void trace(T &&msg) +inline void trace(const T &msg) { - default_logger_raw()->trace(std::forward(msg)); + default_logger_raw()->trace(msg); } template -inline void debug(T &&msg) +inline void debug(const T &msg) { - default_logger_raw()->debug(std::forward(msg)); + default_logger_raw()->debug(msg); } template -inline void info(T &&msg) +inline void info(const T &msg) { - default_logger_raw()->info(std::forward(msg)); + default_logger_raw()->info(msg); } template -inline void warn(T &&msg) +inline void warn(const T &msg) { - default_logger_raw()->warn(std::forward(msg)); + default_logger_raw()->warn(msg); } template -inline void error(T &&msg) +inline void error(const T &msg) { - default_logger_raw()->error(std::forward(msg)); + default_logger_raw()->error(msg); } template -inline void critical(T &&msg) +inline void critical(const T &msg) { - default_logger_raw()->critical(std::forward(msg)); + default_logger_raw()->critical(msg); } } // namespace spdlog diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 6cb0c44d..2dd4958b 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -68,29 +68,3 @@ endif() if(SPDLOG_BUILD_TESTS_HO OR SPDLOG_BUILD_ALL) spdlog_prepare_test(spdlog-utests-ho spdlog::spdlog_header_only) endif() - -# Set up compilation failure test case (only available if compiler supports relaxed constexpr and c++ standard is > 11) -set(HAVE_CXX_RELAXED_CONSTEXPR) -if(CMAKE_CXX_STANDARD GREATER 11) # If we're in c++11 mode we don't have relaxed constexpr, even if our compiler is new enough. - list(FIND CMAKE_CXX_COMPILE_FEATURES "cxx_relaxed_constexpr" HAVE_CXX_RELAXED_CONSTEXPR) -endif() -if(HAVE_CXX_RELAXED_CONSTEXPR AND (SPDLOG_BUILD_FAILING_TESTS OR SPDLOG_BUILD_ALL)) - message(STATUS "Enabling negative compilation unit test target") - set(SPDLOG_FAIL_COMPILATION_TARGET spdlog_fail_compilation_utests) - add_executable(${SPDLOG_FAIL_COMPILATION_TARGET} - test_compilation_failures.cpp - main.cpp) - spdlog_enable_warnings(${SPDLOG_FAIL_COMPILATION_TARGET}) - target_link_libraries(${SPDLOG_FAIL_COMPILATION_TARGET} PRIVATE spdlog::spdlog) - set_target_properties(${SPDLOG_FAIL_COMPILATION_TARGET} PROPERTIES - EXCLUDE_FROM_ALL TRUE - EXCLUDE_FROM_DEFAULT_BUILD TRUE) - add_test(NAME ${SPDLOG_FAIL_COMPILATION_TARGET} - COMMAND ${CMAKE_COMMAND} --build . --target ${SPDLOG_FAIL_COMPILATION_TARGET} --config $ - WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) - set_tests_properties(${SPDLOG_FAIL_COMPILATION_TARGET} PROPERTIES PASS_REGULAR_EXPRESSION "invalid type specifier") - - add_custom_target(${SPDLOG_FAIL_COMPILATION_TARGET}_run_target - COMMAND ${CMAKE_CTEST_COMMAND} -R ${SPDLOG_FAIL_COMPILATION_TARGET} --output-on-failure - COMMENT "Running tests that fail to compile.") -endif() diff --git a/tests/test_compilation_failures.cpp b/tests/test_compilation_failures.cpp deleted file mode 100644 index 640adbdb..00000000 --- a/tests/test_compilation_failures.cpp +++ /dev/null @@ -1,14 +0,0 @@ -#include "includes.h" - -TEST_CASE("{fmt} FMT_STRING functionality preserved (negative test)", "[fmt][fail][fail compilation]") -{ - std::ostringstream oss; - auto oss_sink = std::make_shared(oss); - - spdlog::set_default_logger(std::make_shared("oss", oss_sink)); - spdlog::default_logger()->set_level(spdlog::level::trace); - - spdlog::info(FMT_STRING("The best part of {{fmt}} is the compile time checking: {:d}"), "I shouldn't compile"); - // This should never be able to compile, so running is a failure. - FAIL("This test case isn't meant to compile, let alone run."); -} diff --git a/tests/test_misc.cpp b/tests/test_misc.cpp index cf43f6f4..1d67e879 100644 --- a/tests/test_misc.cpp +++ b/tests/test_misc.cpp @@ -3,7 +3,7 @@ #include "spdlog/fmt/bin_to_hex.h" template -std::string log_info(T &&what, spdlog::level::level_enum logger_level = spdlog::level::info) +std::string log_info(const T &what, spdlog::level::level_enum logger_level = spdlog::level::info) { std::ostringstream oss; @@ -12,7 +12,7 @@ std::string log_info(T &&what, spdlog::level::level_enum logger_level = spdlog:: spdlog::logger oss_logger("oss", oss_sink); oss_logger.set_level(logger_level); oss_logger.set_pattern("%v"); - oss_logger.info(std::forward(what)); + oss_logger.info(what); return oss.str().substr(0, oss.str().length() - strlen(spdlog::details::os::default_eol)); } @@ -269,34 +269,3 @@ TEST_CASE("default logger API", "[default logger]") spdlog::drop_all(); spdlog::set_pattern("%v"); } - -TEST_CASE("{fmt} FMT_STRING functionality preserved (positive test)", "[fmt]") -{ - std::ostringstream oss; - auto oss_sink = std::make_shared(oss); - - spdlog::set_default_logger(std::make_shared("oss", oss_sink)); - spdlog::default_logger()->set_level(spdlog::level::trace); - spdlog::set_pattern("%v"); - - const std::string expected_output_all( - std::string("The best part of {fmt} is the compile time checking: 42") + std::string(spdlog::details::os::default_eol)); - spdlog::trace(FMT_STRING("The best part of {{fmt}} is the compile time checking: {:d}"), 42); - REQUIRE(oss.str() == expected_output_all); - oss.str(""); - spdlog::debug(FMT_STRING("The best part of {{fmt}} is the compile time checking: {:d}"), 42); - REQUIRE(oss.str() == expected_output_all); - oss.str(""); - spdlog::info(FMT_STRING("The best part of {{fmt}} is the compile time checking: {:d}"), 42); - REQUIRE(oss.str() == expected_output_all); - oss.str(""); - spdlog::warn(FMT_STRING("The best part of {{fmt}} is the compile time checking: {:d}"), 42); - REQUIRE(oss.str() == expected_output_all); - oss.str(""); - spdlog::error(FMT_STRING("The best part of {{fmt}} is the compile time checking: {:d}"), 42); - REQUIRE(oss.str() == expected_output_all); - oss.str(""); - spdlog::critical(FMT_STRING("The best part of {{fmt}} is the compile time checking: {:d}"), 42); - REQUIRE(oss.str() == expected_output_all); - oss.str(""); -}