Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
scramsby wants to merge 1 commit into from
Closed

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

scramsby wants to merge 1 commit into from

Conversation

@scramsby
Copy link
Contributor

@scramsby scramsby commented Mar 12, 2020 •
edited
Loading

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

scramsby commented Mar 12, 2020

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.

vitaut
vitaut requested changes Mar 12, 2020
View reviewed changes
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.

@scramsby
Copy link
Contributor Author

scramsby commented Mar 12, 2020

Added unit test when building with C++17.

vitaut
vitaut requested changes Mar 13, 2020
View reviewed changes
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.

@scramsby scramsby force-pushed the fix-fmt-string branch 3 times, most recently from 8e074a9 to 5e2476e Compare March 18, 2020 20:47
vitaut
vitaut requested changes Mar 19, 2020
View reviewed changes
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.

@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

scramsby commented Mar 23, 2020

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

Reviewers

@vitaut vitaut vitaut requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@scramsby @vitaut