Conversation
|
realized logger has capital function names. Do you want me to change to |
|
Yes please, Google style FTW |
| Log(LogLevel::kERROR, std::move(str)); | ||
| } | ||
| inline void Fatal(std::string&& str) { | ||
| Log(LogLevel::kFATAL, std::move(str)); |
There was a problem hiding this comment.
There isn't really a benefit to these rvalue overloads when the underlying function does not swallow up that object and thus can only really use it as a const-ref.
| if (!_enabled || LogLevel::kVERBOSE_DEBUG < min_level_) { | ||
| return; | ||
| } | ||
| Log(LogLevel::kVERBOSE_DEBUG, fmt::format(fmt, std::forward<Ts>(args)...)); |
There was a problem hiding this comment.
If this file is going to be using fmt, it needs to include the relevant headers and dependency in the bazel target.
| if (!_enabled || LogLevel::kDBG < min_level_) { | ||
| return; | ||
| } | ||
| Log(LogLevel::DBG, fmt::format(fmt, std::forward<Ts>(args)...)); |
There was a problem hiding this comment.
It's a weird that when the user calls Log(lvl, str, a, b, c) (i.e., the vararg version) it gets formatted, under-the-hood, with the "printf" formatting, but then, if they call Info(str, a, b, c) it gets formatted with the python-style fmt specifications.
N.B.: I much prefer the fmt version, but I think it needs to be consistent.
There was a problem hiding this comment.
I like the idea of this. In fact, in my own company's logging library, we have similar perfect forwarding of the fmt+args down to the logging functions to present the user with a similarly convenient syntax.
One big design feature that I would very much recommend (and might be able to provide the code for), is to perfectly forward all the way down to the implementation of Log / VLog functions because that will allow for logging functions that do not require any dynamic memory allocation, which is very handy in real-time and signal handler contexts, and quite easy to do with the fmt library. I did this very easily, and I can remove malloc/free in some contexts and still log messages to stdout/stderr.
But, this will require rewriting Log/VLog completely. So, maybe something for @dallison to think about.
There was a problem hiding this comment.
thats a good call. Will update. Thanks
3 participants
Added convience function which avoid str format until after log has been checked.
goes to