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

Using std::source_location. #2667

Open
wants to merge 7 commits into
base: v1.x
Choose a base branch
from

Conversation

gian21391
Copy link

@gian21391 gian21391 commented Mar 7, 2023

It would be great to have support for std::source_location without using the macro version when compiling for C++ 20 (related issue at #1823).

I drafted an idea on how to implement that.

The only issue with this implementation is that it would introduce a small breaking change:

spdlog::info(1)

would not compile anymore. However, that does not work with fmt anyway.

@gian21391 gian21391 changed the title Starting experimenting with std::source_location. Using std::source_location. Mar 7, 2023
@gabime
Copy link
Owner

gabime commented Mar 8, 2023

Thanks. To add this fully means duplicating all of this for all levels (trace(), debug(), warn(), error(), critical()) in spdlog.h AND in logger.h class declarations.

We need to find a way to achieve this without bloating the code. Not sure how..

… in the std library used.

Implementation extended to all logging levels.
Readded the possibility of using spdlog::info(123).
@gian21391
Copy link
Author

I have now extended the implementation to all the other logging levels and managed to reintroduce support for code like spdlog::info(1). Unfortunately, I don't think it's possible to extend this to the logger class due to the impossibility of having a default parameter (or a parameter in general) after a parameter pack in functions and I don't see yet how the same work-around could be applied there. For that, I guess we have to wait until support for non-terminal parameter pack is added to the standard.

Added overloading for std::source_location in logger class.
@gian21391 gian21391 marked this pull request as ready for review March 10, 2023 13:50
@mguludag
Copy link
Contributor

Hello all,
I found a workaround for passing source_location on variadic info, debug,... functions. I wrap the format_string and added source_location as second ctor parameter that solves the issue.
Example:

// common.h
template<typename T>
struct format_string_wrapper
{
    format_string_wrapper(const char* fs, spdlog::details::source_location loc = spdlog::details::source_location::current())
        : format_string_{fs}
        , loc_{loc}
    {}
    operator T()
    {
        return format_string_;
    }
    T format_string_;
    spdlog::details::source_location loc_;
};

template<typename... Args>
using format_string_t = format_string_wrapper<fmt::format_string<Args...>>;

//logger.h
template<typename... Args>
void log(source_loc loc, level::level_enum lvl, format_string_t<Args...> fmt, Args &&... args)
{
    log_(loc, lvl, details::to_string_view(fmt.format_string_), std::forward<Args>(args)...);
}

@JadeMatrix
Copy link

Just wanted to second the approach @mguludag posted — I've been using something similar for a few years in various codebases at work. I actually found this pull request when searching around to see if anyone else had come up with this idea. I have a very basic implementation here that I've used for some personal projects.

Unfortunately a defaulted source_location argument after a function parameter pack doesn't work; the function signature is never matched unless the parameter pack contains 0 items or only a source_location (which defeats the purpose).

An alternate approach is a "double call" syntax, something like handle.warning()("message: {}", msg);. warning would be a function that captures log context (potentially re-usable and/or overridable) and returns a callable context object; the context when called does the actual logging. I don't think this looks as clean so it's not the direction I went. (Note that you wouldn't want to pass the format string in the context parens, as then users are likely to forget to add the second required parens when logging a non-formatted string.)

@mguludag
Copy link
Contributor

mguludag commented Aug 6, 2023

Just wanted to second the approach @mguludag posted — I've been using something similar for a few years in various codebases at work. I actually found this pull request when searching around to see if anyone else had come up with this idea. I have a very basic implementation here that I've used for some personal projects.

Unfortunately a defaulted source_location argument after a function parameter pack doesn't work; the function signature is never matched unless the parameter pack contains 0 items or only a source_location (which defeats the purpose).

An alternate approach is a "double call" syntax, something like handle.warning()("message: {}", msg);. warning would be a function that captures log context (potentially re-usable and/or overridable) and returns a callable context object; the context when called does the actual logging. I don't think this looks as clean so it's not the direction I went. (Note that you wouldn't want to pass the format string in the context parens, as then users are likely to forget to add the second required parens when logging a non-formatted string.)

I implemented source_location into spdlog but I removed extra default source_location parameters from spdlog's templated non format_str log functions. Using log functions with format_string the source_location feature works fine.
#2690

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.

4 participants