Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: replace SPDLOG_* macros with constexpr funcs + source_location #1884

Closed
wants to merge 1 commit into from
Closed

WIP: replace SPDLOG_* macros with constexpr funcs + source_location #1884

wants to merge 1 commit into from

Conversation

franklange
Copy link

@franklange franklange commented Mar 19, 2021

I wanted to investigate what it would take to replace the existing log macros (eg. SPDLOG_TRACE) with constexpr functions that make use of the new std::source_location [1] that will be available in GCC11 [2], as originally discussed here:
#1823

The idea of std::source_location is that you can provide it as a default-value parameter for your log function and it will still log all the info from the call-site.
However, combining default-value params with variadic template functions is almost impossible or at least quite involved. I opted for using user-defined deduction guides [3] as advocated on SO [4].

This is more an experiment but please let me know what you think :)
Would this be a direction worth pursuing?


[1] https://en.cppreference.com/w/cpp/utility/source_location
[2] https://en.cppreference.com/w/cpp/compiler_support
[3] https://en.cppreference.com/w/cpp/language/class_template_argument_deduction#User-defined_deduction_guides
[4] https://stackoverflow.com/questions/57547273/how-to-use-source-location-in-a-variadic-template-function

Adds constexpr functions as alternatives to the log macros which allows to use std::source_location

spdlog_critical(">>>CRITICAL");
spdlog_logger_critical(spdlog::default_logger_raw(), ">>>CRITICAL");

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This compiled and worked for me on Linux + GCC 10.2.0.

@franklange franklange changed the title WIP: use source_location for SPDLOG_* macros WIP: replace SPDLOG_* macros with constexpr functions + source_location Mar 19, 2021
@franklange franklange changed the title WIP: replace SPDLOG_* macros with constexpr functions + source_location WIP: replace SPDLOG_* macros with constexpr funcs + source_location Mar 19, 2021
@@ -26,7 +26,7 @@ endif ()
# Compiler config
# ---------------------------------------------------------------------------------------
if (NOT CMAKE_CXX_STANDARD)
set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD 17)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently needed for deduction guides.

@gabime
Copy link
Owner

gabime commented Mar 19, 2021

Interesting approach. Thanks, for sharing. I wonder if the struct trick is the only way to achieve this. Ideally, this would be added directly to the existing functions.

@franklange
Copy link
Author

Interesting approach. Thanks, for sharing. I wonder if the struct trick is the only way to achieve this. Ideally, this would be added directly to the existing functions.

Heyhey, thanks for the quick reply!
Unfortunately I couldn't find any other way of combining a variadic template with a default-value parameter. So I guess this might be the way to go for all the fmt-style functions.

Regarding bigger picture: so you envision using source_location for all the log functions? eg:

// spdlog.h

template<typename T>
inline void trace(const T &msg, std::source_location loc = std::source_location::current())
{
    default_logger_raw()->trace(loc, msg); // needs mods in logger.h to take + forward source_location
}

I wasn't quite sure if it's desirable to "force" logging the source location in every log function. If I understand the API correctly then currently having these additional macros allows me to chose wether I'm interested in src_location or not.

So ya, let me know what you think. I can also add this to the existing functions but then I'd have to adjust logger.h as well.

@gabime
Copy link
Owner

gabime commented Mar 24, 2021

So ya, let me know what you think. I can also add this to the existing functions but then I'd have to adjust logger.h as well.

If the only benefit is that one can call spdlog_trace instead of SPDLOG_TRACE, then I don't think it worth the extra complexity (not to mention it only supported in c++20).

Also, the macros are zero code if the level is disabled at compile time, so they can't be completely replaced with the proposed.

@franklange
Copy link
Author

franklange commented Mar 24, 2021

Also, the macros are zero code if the level is disabled at compile time, so they can't be completely replaced with the proposed.

Shouldn't the if constexpr inside the functions here make sure that they're empty and optimized away if the corresponding level is disabled?

I don't mind throwing this away. I started this purely based on your "thumbs up" on the original idea posted here: #1823

What did you have in mind there?

@gabime
Copy link
Owner

gabime commented Mar 25, 2021

Shouldn't the if constexpr inside the functions here make sure that they're empty and optimized away if the corresponding level is disabled?

Its not exactly the same. For example the args to the function would still be evaluated (spdlog_debug(“{}”, some_expensive_func());

What did you have in mind there?

That should be considered, but like I said, it seems to not worth it in the current state of affairs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants