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

use std::source_location and replace macros with constexpr functions #1823

Open
L0ric0 opened this issue Feb 8, 2021 · 4 comments
Open

use std::source_location and replace macros with constexpr functions #1823

L0ric0 opened this issue Feb 8, 2021 · 4 comments

Comments

@L0ric0
Copy link

L0ric0 commented Feb 8, 2021

c++ 20 added std::source_location which provides the current file, line, collumn and function name. as I understand the SPDLOG_info ... macros are only needed to get this informations, so it would be nice to replace the macros with constexpr functions for c++ 20 code (std::source_location is currently supported on gcc trunk)

@franklange
Copy link

Experimental PR was closed and revealed the following issues:

  1. Default use of std::source_location means adding it as a default parameter to the log function. If the log function is a variadic template of the form log(fmtstr, args...) adding a default parameter is non-trivial.
  2. Replacing the macros with constexpr functions makes no sense because ultimately they will call logger->log(), ie a non-constepxr function. That only leaves using if constexpr () within the function but that still would mean that function arguments would be evaluated which they are not in case of disabled macros.
  3. It's unclear whether it makes sense to keep the macros and rather add std::source_location to the existing non-macro log functions. That would mean always logging source_location info, whereas currently the caller can choose whether they want location information or not by using either the functions or the macros.

So overall I wouldn't know how to add std::source_location to this project without disrupting the existing design.

@sylveon
Copy link
Contributor

sylveon commented Apr 19, 2021

I think we should at least replace spdlog::source_loc by std::source_location when it's available, to make consumption from C++20 nicer.

@oct15demo
Copy link

oct15demo commented Mar 18, 2022

#define fl_debug(...) log(spdlog::source_loc{FILE, LINE, static_cast<const char *>(FUNCTION)}, spdlog::level::trace, VA_ARGS)

Just using the above gives you the option of not using SPDLOG_DEBUG to see filename:line# without changing anything in the current code base. You get individual loggers, not just the default, with the file number, line number, handy in development when you want console output to link to the source code.

I went a bit further and changed the existing level function names in spdlog.h and logger.h for my own use. Plus some dereferencing the pointer to give me an easy syntax logger.debug("my message"); That part could probably be made configurable, and is just syntactic sugar.

But getting back to the marco above, perhaps you could consider offering that in an example, or footnote to the formatting table for %s and %#.

@USAFrenzy
Copy link

USAFrenzy commented Apr 15, 2022

I'm very much still in the process of learning and have started writing my own version of a logging library inspired by this one; when I went to add a source flag with some enum flags to use as specifiers, I ran into this issue for getting accurate call sites until I came across this post on SO ( https://stackoverflow.com/a/66402319/11410972 ). In a tight loop of 1,000,000 iterations, the CPU cycles used to store the message and the std::source_location data was pretty negligible and had no real noticeable signs on performance on the scale of microseconds (it was about a 200 nanosecond drop on my machine). The added benefit being that since spdlog calls msg.source and msg.payload, it may even be a 1 to 1 drop in. I don't know if this may be of any help but thought I'd put this out there as I ironically came across this issue in the search results when working with std::source_location. This idea only addresses point 1 of franklange's comment though. Editing as I just saw this point was addressed in #1959; please disregard this comment

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

No branches or pull requests

6 participants