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

Enable FMT_STRING() use with types other than string literals #1589

Closed
wants to merge 1 commit into from

Conversation

scramsby
Copy link
Contributor

@scramsby scramsby commented Mar 12, 2020

This enables using FMT_STRING() with raw character arrays, like:

constexpr char msg[5] = {'h', 'e', 'l', 'l', 'o'};
FMT_STRING(msg);

...and other string view types like:

constexpr std::string_view msg{"hello"};
FMT_STRING(msg);

Also fixes some constexpr evaluation build errors when building with C++17 with arg_ref internal class.

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

@vitaut
Copy link
Contributor

vitaut commented Mar 12, 2020

Thanks for the PR. Could you explain the motivation for this change?

@scramsby
Copy link
Contributor Author

Our coding standards use string_view and equivalent types for defining shared string constants as they are generally more type safe. This enables us to continue to use such types for use in our logging system which uses fmt. The change in the terminating null character detection is mostly just because I noticed it while working in the same area, we don't have concrete need for it currently.

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Please add unit tests to test/format-test.cc and address inline comments.

include/fmt/format.h Outdated Show resolved Hide resolved
include/fmt/format.h Outdated Show resolved Hide resolved
include/fmt/format.h Outdated Show resolved Hide resolved
@scramsby
Copy link
Contributor Author

Added unit test when building with C++17.

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more comments, otherwise looks good.

include/fmt/format.h Outdated Show resolved Hide resolved
include/fmt/format.h Outdated Show resolved Hide resolved
@scramsby scramsby force-pushed the fix-fmt-string branch 3 times, most recently from 8e074a9 to 5e2476e Compare March 18, 2020 20:47
Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some build failures in CI.

include/fmt/core.h Outdated Show resolved Hide resolved
include/fmt/core.h Outdated Show resolved Hide resolved
@scramsby scramsby force-pushed the fix-fmt-string branch 3 times, most recently from 729032b to 45c60ad Compare March 23, 2020 20:52
@scramsby
Copy link
Contributor Author

Finally got the changes trimmed down to what is needed and fixed the last few niggling platform-specific build errors, so this should be good to go absent further feedback.

@vitaut
Copy link
Contributor

vitaut commented Mar 24, 2020

Merged with minor tweaks in 664dd88. Thanks!

@vitaut vitaut closed this Mar 24, 2020
@scramsby scramsby deleted the fix-fmt-string branch March 24, 2020 21:16
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