Skip to content

Make format work with C++17 std::string_view #571

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
Merged
vitaut merged 1 commit into from
Sep 20, 2017
Merged

Make format work with C++17 std::string_view #571

vitaut merged 1 commit into from
Sep 20, 2017

Conversation

@feroldi
Copy link
Contributor

@feroldi feroldi commented Sep 17, 2017

Enable std::string_view to work with format when compiling with C++17.

Change log:

  • fmt::StringRef is constructible from std::string_view.
  • fmt::StringRef is convertible to std::string_view (only explicitly)[1].
  • fmt::format accepts std::string_view as an argument for the argument list.

[1] This is open to debate. Should it be an implicit conversion, or should it not exist at all? The argument in favor of it is that std::string_view is now the common string reference, and all other string implementations could take advantage of it by providing a conversion to it. A counter-argument would be that fmtlib prefers fmt::StringRef as the common string reference.

Make format work with C++17 std::string_view
3b7449d 
Tests for C++17 std::string_view
@feroldi feroldi mentioned this pull request Sep 17, 2017
Closed
@vitaut vitaut merged commit 2a619d9 into fmtlib:master Sep 20, 2017
@vitaut
Copy link
Contributor

vitaut commented Sep 20, 2017

Looks great, thanks for the PR!

Should it be an implicit conversion, or should it not exist at all?

Conversion seems fine unless it causes some issues for existing code such as overload ambiguity. Eventually StringRef will be replace with std::string_view anyway.

@mridgewell
Copy link

mridgewell commented Oct 8, 2017

Thanks for this, but it doesn't work in Visual Studio because the __cplusplus definition is out of date in Microsoft's compiler. I suggest either removing that check or adding a specific one for _MSC_VER.

@vitaut
Copy link
Contributor

vitaut commented Oct 8, 2017

@mridgewell If you are interested in string_view support on MSVC, could you submit a PR adding a version check specific to this compiler to

#if FMT_HAS_INCLUDE(<string_view>) && __cplusplus > 201402L

?

@feroldi
Copy link
Contributor Author

feroldi commented Oct 8, 2017

Removing the check for __cplusplus prevents GCC from compiling, even if you're not making use of string_view in your own code. There's no issue with Clang though, as it accepts <string_view> in any standard version. I'm not sure about MSVC.

@mridgewell
Copy link

mridgewell commented Oct 9, 2017

I wouldn't mind, but looks like @mwinterb has already done the work, which solves my problem :)
Thanks!

vitaut added a commit that referenced this pull request Feb 7, 2018
@vitaut
Make format work with C++17 std::string_view (#571)
bd506f0
vitaut added a commit that referenced this pull request Feb 7, 2018
@vitaut
Make format work with C++17 std::string_view (#571)
67e070f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@feroldi @vitaut @mridgewell