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

[source_loc] support for owning strings #2867

Closed
timblechmann opened this issue Aug 27, 2023 · 12 comments · May be fixed by #2935
Closed

[source_loc] support for owning strings #2867

timblechmann opened this issue Aug 27, 2023 · 12 comments · May be fixed by #2935

Comments

@timblechmann
Copy link
Contributor

timblechmann commented Aug 27, 2023

source_loc does not own the strings for file name and function name. this is a perfectly reasonable default in most use cases where this information is filled by the preprocessor.

however i'm running into a funny situation when trying to re-direct qt's logging to spdlog and using multithreaded logging:


i've been meditating a bit about this. it's a bit of an edge case, but it would be great if spdlog could provide a second api similar to source_loc, that uses owning std::string to allow applications to fill a source_loc from source locations other than c/c++

@tt4g
Copy link
Contributor

tt4g commented Aug 28, 2023

First, unless you are using spdlog::async_logger, logs are output synchronously, so the difference between std::string and char * does not matter.

IMO.
source_loc is used to access the __FILE__ and __LINE__ macros.
These macros are provided to hold the location information of the source code from which spdlog logging functions are called.
The copy cost incurred by replacing the compile-time constant char * with std::string cannot be ignored.

As far as I can see QMessageLogContext is an object to transfer log data generated elsewhere to another location.
The location of that log data is not the spdlog logging function call location.
Therefore, you should not use source_log, but should output the log message as additional information to the log forwarded from another location.

@timblechmann
Copy link
Contributor Author

@tt4g, the analysis is basically the same conclusion that i came to: it would require a separate API in order to support source_loc that cannot be filled from compile-time constants (not an uncommon use case when working with embedded scripting languages). in a "good old c/c++ codebase", this is not an issue

@tt4g
Copy link
Contributor

tt4g commented Aug 28, 2023

spdlog uses the fmt library to format log messages.
Since fmt can format user-defined types (https://fmt.dev/9.1.0/api.html#formatting-user-defined-types), You can define template <> struct fmt::formatter<QMessageLogContext> and print QMessageLogContext as the log argument.
e.g. spdlog::info("Qt source info {}, context);

@gabime
Copy link
Owner

gabime commented Aug 28, 2023

spdlog uses the fmt library to format log messages.
Since fmt can format user-defined types (https://fmt.dev/9.1.0/api.html#formatting-user-defined-types), You can define template <> struct fmt::formatter and print QMessageLogContext as the log argument.
e.g. spdlog::info("Qt source info {}, context);

Right, other ways would require significant modifications to the API.

The fmt formatting is done in the log call site so it would work even if the QMessageLogContext get destroyed before the the async worker thread processes the message. See here for simple example for user defined types.

@timblechmann
Copy link
Contributor Author

formatting a QMessageLogContext would be the low-hanging fruit, but it prevents using formatting patterns for the file location: https://github.com/gabime/spdlog/wiki/3.-Custom-formatting#pattern-flags or am i missing something?

Right, other ways would require significant modifications to the API.

yes, this is more a feature request in order to ease the integration of spdlog with source location provided from a vm

@gabime
Copy link
Owner

gabime commented Aug 28, 2023

https://github.com/gabime/spdlog/wiki/3.-Custom-formatting#pattern-flags or am i missing something?

Custom formatter would execute in the async worker thread, so it could be done but care is needed in that regard.

@timblechmann
Copy link
Contributor Author

Custom formatter would execute in the async worker thread, so it could be done but care is needed in that regard.

from my reading of the code, the filename formatters like %@ directly access source_loc 🤔 ... how can i point this to a different object with a custom formatter?


that said: i wonder if the async formatters could ensure that the ownership of the strings that source_loc point to? there are a few other edge cases apart from using VMs, e.g. if __FILE__ / __LINE__ / std::source_location point to memory that's unmapped before the async worker thread accesse them (thinking about dlclose and friends)

@gabime
Copy link
Owner

gabime commented Aug 28, 2023

from my reading of the code, the filename formatters like %@ directly access source_loc 🤔 ... how can i point this to a different object with a custom formatter?

With the custom formatter you would need to implement some kind of mechanism for this.I am pretty sure it doesn't worth it. Easiest would be implementing user defined type.

@gabime gabime closed this as completed Sep 13, 2023
@timblechmann
Copy link
Contributor Author

@gabime, as this ticket is closed as "completed", is there any change that i need to cherry-pick? i've been working on a patch to async_msg to take ownership of these strings, so i wonder if that's obsolete now

@gabime
Copy link
Owner

gabime commented Sep 14, 2023

It was closed because there is no simple fix for this and the need for this feature is rare to be worth it.

@timblechmann
Copy link
Contributor Author

would you accept a patch that enables this feature via a preprocessor define?

@gabime
Copy link
Owner

gabime commented Sep 15, 2023

I prefer not to add complexity to the codebase for such a rare feature.

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 a pull request may close this issue.

3 participants