Add std::string_view constructors to wxString (PR #23711)
Ian McInerney
This adds two new constructors to wxString to create a string from a std::string_view or a std::wstring_view. Since these are only present in C++17, I have added the define wxHAS_STD_STRING_VIEW that tests if the header and functionality exists in the standard library, and then enables the constructors accordingly. Since a string view is just a data pointer and the length, everything can be done through calls to assign in the header, so there are no changes to the cpp file. I believe that this means that the consumer program can still use the string view constructors even if wx is compiled with a C++ version before C++17.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/23711
Commit Summary
- 6cae616 Add std::string_view constructors to wxString
File Changes
(2 files)
- M include/wx/string.h (22)
- M tests/strings/stdstrings.cpp (23)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/23711.patch
- https://github.com/wxWidgets/wxWidgets/pull/23711.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ian McInerney
@imciner2 pushed 1 commit.
- bf8ed75 Add std::string_view constructors to wxString
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ian McInerney
@imciner2 pushed 1 commit.
- 40f9c0f Add std::string_view constructors to wxString
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ian McInerney
Realized I forgot to add docs for these methods, so I have just done that.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Nice, thanks!
I just wonder if we shouldn't provide FromUTF8() overload taking std::string_view too, especially as the docs refer to it?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ian McInerney
I just wonder if we shouldn't provide FromUTF8() overload taking std::string_view too, especially as the docs refer to it?
Good point, that was a part that I thought was more automatic and didn't realize it needs explicit calls. I assume we need both the checked and unchecked functions?
I think I can just piggyback then off of the existing functions even, just decomposing the view into its data and calling the existing ones like this:
static wxString FromUTF8Unchecked(std::string_view view)
{
return wxString::FromUTF8Unchecked(view.data(), view.length());
}
static wxString FromUTF8(std::string_view view)
{
return wxString::FromUTF8(view.data(), view.length());
}
(does that look right?)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I just wonder if we shouldn't provide FromUTF8() overload taking std::string_view too, especially as the docs refer to it?
Good point, that was a part that I thought was more automatic and didn't realize it needs explicit calls. I assume we need both the checked and unchecked functions?
Probably yes, for consistency.
I think I can just piggyback then off of the existing functions even, just decomposing the view into its data and calling the existing ones like this:
static wxString FromUTF8Unchecked(std::string_view view) { return wxString::FromUTF8Unchecked(view.data(), view.length()); } static wxString FromUTF8(std::string_view view) { return wxString::FromUTF8(view.data(), view.length()); }(does that look right?)
Yes, it does, thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ian McInerney
@imciner2 pushed 1 commit.
- a487399 fixup! Add std::string_view constructors to wxString
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ian McInerney
Ok, I have pushed the new UTF8 conversions as a fixup commit for review.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Sorry, I didn't think about it, but this results in multiple
warning STL4038: The contents of <string_view> are available only with C++17 or later.
when building with MSVS, see e.g. this build log.
We already had one such warning in string.cpp for charconv, which was already not ideal, but now we're going to have them for every wx/string.h inclusion which is much worse. We need some way to avoid this, probably by adding a special MSVC-specific wxHAS_CXX17_INCLUDE macro or something like this which would evaluate to false for MSVC with __has_include but not in C++17 mode...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ian McInerney
We already had one such warning in string.cpp for charconv, which was already not ideal,
It looks like the charconv header is included two different ways actually. The main library has the test for __has_include(<charconv>), but the tests that use it instead do wxCHECK_CXX_STD(201703L). The latter is defined to only return true when the C++ version reported by the compiler is at least C++17.
So I guess the fix for both the warnings here and the other charconv warning is to instead use wxCHECK_CXX_STD(201703L) in place of the _has_include check (since I think we can assume if we have C++17 then the include will always be there).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Unfortunately as the comment in string.cpp says, g++ 7 claims C++17 support but doesn't provide <charconv>. So it really looks like we need 2 different ways of testing for it, for MSVC and for gcc.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I've added the macro I meant in #23731 and cherry picked your commit modified to use it there. Please let me know if you see anything wrong, otherwise I'll merge that PR soon. TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ian McInerney
@imciner2 pushed 1 commit.
- 3ca66d1 Add new macro for standard library header inclusion
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ian McInerney
Well, I've just pushed how I would have implemented it (and it is a bit cleaner IMO).
I was always planning on coding this up on Monday morning once I got to the computer that already had the branch checked out and built, and I really didn't see why there was a particular rush to do the changes (there was no discussion of an imminent 3.3.0 release that this was holding up, and no notice that you were becoming impatient with this PR and the code in it). I don't see why you felt the need to jump in and take over the PR when I was being responsive to the comments you were providing. I am always mindful of people's time, so I am not pushy when wanting reviews for PRs (not once since proposing this PR 2 weeks ago did I do any pings or bumps), so I don't see why you couldn't wait for me to do this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Well, I've just pushed how I would have implemented it (and it is a bit cleaner IMO).
I was always planning on coding this up on Monday morning once I got to the computer that already had the branch checked out and built, and I really didn't see why there was a particular rush to do the changes (there was no discussion of an imminent 3.3.0 release that this was holding up, and no notice that you were becoming impatient with this PR and the code in it). I don't see why you felt the need to jump in and take over the PR when I was being responsive to the comments you were providing. I am always mindful of people's time, so I am not pushy when wanting reviews for PRs (not once since proposing this PR 2 weeks ago did I do any pings or bumps), so I don't see why you couldn't wait for me to do this.
Sorry Ian, I really didn't want to upset you, I've just switched to MSVS 2022 for one of the projects I'm currently working on and got tired of seeing the warning about charconv during every rebuild, so I decided to go ahead and fix it when I had a bit of time yesterday. I didn't know you had already done it, of course, and on the contrary thought that I would save you some time and, precisely because your PRs are always so good and you're so patient about not pinging me even when I let them sit here for a long time, I wanted to compensate by doing things quickly for once. Sorry that it turned out in the way it did, once again it certainly wasn't my intention.
Anyhow, I'll gladly merge your version instead, I just wonder if we could rename the new macro for wxHAS_CXX17_INCLUDE, as this reads a bit better to me and seems more consistent with the other wxHAS_XXX macros. It's not a big deal, of course, so if you really prefer your version, we could keep it too.
Sorry again for the mix-up with the PRs.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ian McInerney
@imciner2 pushed 1 commit.
- b8d3b37 Add new macro for standard library header inclusion
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ian McInerney
I just wonder if we could rename the new macro for wxHAS_CXX17_INCLUDE
Done. I had thought of the first one as mimicking the C++ version (since it replaces __has_include, I tried to keep it looking like that), but standardizing to wx is probably better in this case.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Great, thanks, will merge soon.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Merged #23711 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.