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

Add missing header needed for recent versions of fmt #95

Closed
wants to merge 1 commit into from

Conversation

jeremyong
Copy link

(Thanks for the nice library btw).

In recent versions of fmtlib/fmt, types related to argument storage moved to fmt/args.h to make the core header leaner. The undefined references to dynamic_arg_storage are fixed with this change when using a more recent fmt library as an external dependency.

@odygrd
Copy link
Owner

odygrd commented Apr 10, 2021

Hey, thanks for the PR. Unfortunately I can not merge this yet because it looks like this args.h is missing from the latest libfmt release 7.1.3 and that will break quill in package managers because the file is not there to be included ( https://github.com/fmtlib/fmt/tree/7.1.3/include/fmt )

An alternative would be to use __has_include before including it but that is a c++17 only feature.

I think we need to wait for the next libfmt release.

As a solution to support backwards compatibility with prior to 7.1.3 fmt versions where args.h file is missing

(for both external/interval versions)

  1. We need to include fmt/core.h first.
  2. Then we need to check -> https://github.com/fmtlib/fmt/blob/7.1.3/include/fmt/core.h#L21
    #if FMT_VERSION > 70103 then include fmt/args.h (or the path to the bundled fmt file)

If you are happy to implement this change I would be happy to merge it, otherwise I will implement the above proposed solution over the next few weeks.

As explained above for this change to have effect and include fmt/args.h it requires FMT_VERSION to be bumped which will happen in the next libfmt release.

@odygrd
Copy link
Owner

odygrd commented Apr 17, 2021

I have implemented the above change here #100

As noted for it to work FMT_VERSION needs to be incremented in fmt/core.h

@jeremyong
Copy link
Author

Thanks!

@odygrd odygrd closed this Apr 17, 2021
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.

2 participants