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

Format string is hard-coded to be a string literal #27

Closed
gh-andre opened this issue Jan 13, 2022 · 5 comments
Closed

Format string is hard-coded to be a string literal #27

gh-andre opened this issue Jan 13, 2022 · 5 comments

Comments

@gh-andre
Copy link

gh-andre commented Jan 13, 2022

If the format string is not a string literal, cryptic compiler messages are reported and at first it's not even clear what the problem is. For example:

template <typename ... T>
void mylog(const std::string_view& format, T&& ... arg)
{
  FMTLOG_ONCE(fmtlog::INF, format, std::forward<T>(arg)...);
}

int main(void)
{
  mylog("test log line {} {}"sv, "ABC", 123);
  return 0;
}

GCC reports use of parameter from containing function against the format parameter and an additional error 'constexpr' call flows off the end of the function in core.h in fmt. VC++ reports just function cannot access 'fmt'.

This is caused by FMT_STRING used in all logging macros, which only works with string literals. Removing FMT_STRING from logging macros fixes these errors.

@MengRao
Copy link
Owner

MengRao commented Jan 14, 2022

Currently only literal format string is supported, you can wrap the log statements using macros simply as logio does.

@gh-andre
Copy link
Author

Yes, that's what I ended up doing - I split FMTLOG_ONCE to allow location supplied as an argument, so it can be propagated through the application, until it reaches fmtlog.

You may want to document in README.md that only string literals are supported for formats. This will make it more clear that for applications that would need to wrap the logger in their own layer it won't work without patching.

Closing this issue, since you consider this working as designed. I will update my v2.1.1 patch at some point for those who's interested in this path.

@gh-andre
Copy link
Author

Implemented the change mentioned above, which allows format in FMTLOG_ONCE to be any string type and also supply source location explicitly, if anyone is interested (commit gh-andre@bd68c0b) on v2-1-1-patches branch.

https://github.com/gh-andre/fmtlog/tree/v2-1-1-patches

@MengRao
Copy link
Owner

MengRao commented Jan 18, 2022

Please check latest update, now FMT_STRING is removed and the format string type is the same as fmt::format/print.

@gh-andre
Copy link
Author

Thanks. I merged my changes with the new code. Here's the branch with all changes, if you want to consider any of it.

https://github.com/gh-andre/fmtlog/tree/main-patches

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

No branches or pull requests

2 participants