-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Replace using of internal fmt lib API with public API #3527
base: 10.11
Are you sure you want to change the base?
Replace using of internal fmt lib API with public API #3527
Conversation
|
5ffe8e8
to
8ba6ff0
Compare
Nice! Thanks for noticing. Can you rebase this back to 10.11? Are the libfmt versions supporting this public API back to the original minimum supported (v 8 something I think)? FYI @ZhongRuoyu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Logic looks good to me.
fmtlib/fmt@6012dc9 seems to be the commit that introduced fmt::dynamic_format_arg_store
. It was then released in 6.2.0, so compatibility-wise it should be okay.
Though, for consistency, I believe cmake/libfmt.cmake
needs to be updated too.
right, let's fix cmake/libfmt.cmake too, it is supposed to test the availability of the API that the code is using, not availability of something completely different |
Good catch, I'll make the change as soon as possible, thanks! |
8ba6ff0
to
0c2119d
Compare
I've updated |
Note the validation of the bundled version is failing - https://buildbot.mariadb.org/#/builders/1/builds/50345
|
Looks like the bundled libfmt is not downloading after my refactor of |
The commit cd5808e introduced a union as a storage for the format argument passed to the internal API fmt::detail::make_arg. This was done to solve the issue that the internal API no longer accepted temporary variables. However, it's generally better to avoid using internal APIs, as they are more likely to have breaking changes in the future. Instead, we can use the public API fmt::dynamic_format_arg_store to dynamically build the argument list. This API accepts temporary variables, and its behavior is more stable than the internal API. `libfmt.cmake` is updated to reflect the change as well. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
0c2119d
to
7f27be6
Compare
I mistake that |
Related MDEV: https://jira.mariadb.org/browse/MDEV-31963
Description
The commit cd5808eb introduced a union as a storage for the format argument passed to the internal API fmt::detail::make_arg. This was done to solve the issue that the internal API no longer accepted temporary variables.
However, it's generally better to avoid using internal APIs, as they are more likely to have breaking changes in the future. Instead, we can use the public API fmt::dynamic_format_arg_store to dynamically build the argument list. This API accepts temporary variables, and its behavior is more stable than the internal API.
How can this PR be tested?
Basing the PR against the correct MariaDB version
PR quality check
Copyright
All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.