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

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

Merged
merged 1 commit into from
Sep 20, 2017
Merged

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.

Tests for C++17 std::string_view
@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

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

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

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.

3 participants