ICU-22843 UnicodeString <-> std::u16string_view / wstring_view #3076

markusicu · 2024-08-01T18:20:23Z

Checklist

Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22843
Required: The PR title must be prefixed with a JIRA Issue number.
Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
Required: Each commit message must be prefixed with a JIRA Issue number.
Issue accepted (done by Technical Committee after discussion)
Tests included, if applicable
API docs and/or User Guide docs changed or added, if applicable

markusicu

Hi @richgillam I got help from @eggrobin who showed me (https://godbolt.org/z/5WMbWszoE) how to templatize the inputs so that in most cases we go from a group of four new overloads to just one templatized one. I like this much better! Do you agree?

See just the third commit for the changes compared with the proposal.

I added a few "WDYT?" comments for both of you.

icu4c/source/common/unicode/unistr.h

icu4c/source/test/intltest/ustrtest.cpp

markusicu · 2024-08-02T23:45:24Z

icu4c/source/i18n/number_decimalquantity.cpp

@@ -1082,7 +1082,7 @@ UnicodeString DecimalQuantity::toScientificString() const {
    result.append(u'E');
    int32_t _scale = upperPos + scale + exponent;
    if (_scale == INT32_MIN) {
-        result.append({u"-2147483648", -1});
+        result.append(UnicodeString(true, u"-2147483648", -1));


FYI
Adding overloads can still break certain call sites, but I think this is rare.
In this case, the old code also copied the contents; now I am aliasing it.

icu4c/source/common/unicode/unistr.h

richgillam

Wow. There's a lot of stuff here I don't understand (because I've never really worked in a modern dialect of C++), but I definitely like the conciseness. You'll have to update the API proposal, of course, and maybe that's a good place to go into a little more detail about what you're actually doing. I was having trouble following it, but maybe that's just me. Regardless, I trust you guys...

icu4c/source/common/unicode/char16ptr.h

jira-pull-request-webhook · 2024-08-03T19:31:39Z

Notice: the branch changed across the force-push!

icu4c/source/i18n/number_decimalquantity.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

icu4c/source/common/unistr.cpp

aheninger · 2024-08-05T01:10:38Z

I like this template-function approach. It's forcing me to learn something new (to me) with std::enable_if

richgillam

I got lost in some of the changes (I still haven't really gotten my head around enable_if), but it all sounds good and it looks like you've done appropriate testing...

markusicu · 2024-08-09T21:51:00Z

I finally finished the unit test code. PTAL

markusicu · 2024-08-09T23:08:44Z

I will look at the MSVC failures :-/

eggrobin · 2024-08-09T23:15:44Z

I will look at the MSVC failures :-/

Since I do my ICU4C work on MSVC, I am happy to look at them next week if you prefer.

icu4c/source/test/intltest/ustrtest.cpp

eggrobin · 2024-08-09T23:32:10Z

icu4c/source/test/intltest/ustrtest.cpp

+
+    UnicodeString aliasFromStr = UnicodeString::readOnlyAlias(str16);
+    assertTrue("aliasFromStr pointer alias", aliasFromStr.getBuffer() == str16.data());
+    assertEquals("aliasFromStr length", (int32_t)str16.length(), aliasFromStr.length());


I would note that we now consistently use static_cast for this since #3060, and that while it is verbose there is some value in not having to think about any of those C casts that might hide nasty stuff…

But really we should just not have to cast here, assertEquals should be templatized and should accept heterogeneous arguments, and then the call site can look unambiguously nicer.

I would note that we now consistently use static_cast for this since #3060, and that while it is verbose there is some value in not having to think about any of those C casts that might hide nasty stuff…

I think that static_cast for simple integer type casts adds a lot more clutter than value.

But really we should just not have to cast here, assertEquals should be templatized and should accept heterogeneous arguments, and then the call site can look unambiguously nicer.

I don't know if templates would let us compare signed and unsigned types unhindered.

We have had a little discussion about switching ICU4C tests to using the Google unit test framework, but someone would have to work out how to do so. Probably only for new test code, which means adding a whole new, very different test suite, and one probably less portable than the primitive one we have.

markusicu · 2024-08-12T16:00:29Z

It looks like I got MSVC to work where the wchar_t version of the test runs. Back to @richgillam & @eggrobin

markusicu · 2024-08-12T16:31:17Z

All tests pass 🎉 , except of course for the single-commit check. I plan to squash after review.

Also, I am considering a follow-up proposal+PR for the same kind of template overload for operator!=. Seems silly not to have done it right away. But I also want to get this PR here in as is.

I am still not planning to add overloads for most of the UnicodeString API, and we should at some point have a discussion about future ICU4C APIs taking string inputs.

richgillam

Tests look good. I had one nit, but I'm fine if you ignore it.

icu4c/source/test/intltest/ustrtest.cpp

richgillam · 2024-08-12T18:43:20Z

Also, I am considering a follow-up proposal+PR for the same kind of template overload for operator!=. Seems silly not to have done it right away.

Ouch! I'm bummed we didn't think of that. I'm fine with that in a follow-up PR, although I kind of wish we'd thought about it in time to get it into this one.

jira-pull-request-webhook · 2024-08-12T19:58:34Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

…mplates

jira-pull-request-webhook · 2024-08-13T15:18:52Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

markusicu commented Aug 2, 2024

View reviewed changes

richgillam previously approved these changes Aug 3, 2024

View reviewed changes

icu4c/source/common/unicode/char16ptr.h Show resolved Hide resolved

markusicu dismissed richgillam’s stale review via cff5f59 August 3, 2024 19:24

markusicu force-pushed the unistr-u16string_view branch from cff5f59 to 9146ba1 Compare August 3, 2024 19:31

markusicu commented Aug 4, 2024

View reviewed changes

icu4c/source/common/unistr.cpp Outdated Show resolved Hide resolved

richgillam previously approved these changes Aug 6, 2024

View reviewed changes

markusicu dismissed richgillam’s stale review via e58f751 August 9, 2024 21:44

markusicu marked this pull request as ready for review August 9, 2024 21:51

markusicu requested review from richgillam and eggrobin August 9, 2024 21:51

eggrobin reviewed Aug 9, 2024

View reviewed changes

icu4c/source/test/intltest/ustrtest.cpp Outdated Show resolved Hide resolved

eggrobin reviewed Aug 9, 2024

View reviewed changes

markusicu requested a review from eggrobin August 12, 2024 16:00

richgillam approved these changes Aug 12, 2024

View reviewed changes

icu4c/source/test/intltest/ustrtest.cpp Show resolved Hide resolved

markusicu force-pushed the unistr-u16string_view branch from 2e1805e to 150466b Compare August 12, 2024 19:58

rp9-next closed this Aug 13, 2024

rp9-next reopened this Aug 13, 2024

rp9-next closed this Aug 13, 2024

rp9-next reopened this Aug 13, 2024

rp9-next closed this Aug 13, 2024

rp9-next reopened this Aug 13, 2024

ICU-22843 UnicodeString <-> std::u16string_view / wstring_view via te…

c8a7897

…mplates

markusicu force-pushed the unistr-u16string_view branch from 150466b to c8a7897 Compare August 13, 2024 15:18

markusicu merged commit 7220649 into unicode-org:main Aug 13, 2024
99 checks passed

markusicu deleted the unistr-u16string_view branch August 13, 2024 16:10

roubert mentioned this pull request Aug 14, 2024

ICU-22843 Simplify UTF-16 string literals #3102

Merged

7 tasks

roubert mentioned this pull request Aug 26, 2024

ICU-22843 Enable constructing UnicodeString from literal in fixed time #3106

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICU-22843 UnicodeString <-> std::u16string_view / wstring_view #3076

ICU-22843 UnicodeString <-> std::u16string_view / wstring_view #3076

markusicu commented Aug 1, 2024 •

edited

Loading

markusicu left a comment

markusicu Aug 2, 2024

richgillam left a comment

jira-pull-request-webhook bot commented Aug 3, 2024

aheninger commented Aug 5, 2024

richgillam left a comment

markusicu commented Aug 9, 2024

markusicu commented Aug 9, 2024

eggrobin commented Aug 9, 2024

eggrobin Aug 9, 2024

markusicu Aug 12, 2024

markusicu commented Aug 12, 2024

markusicu commented Aug 12, 2024

richgillam left a comment

richgillam commented Aug 12, 2024

jira-pull-request-webhook bot commented Aug 12, 2024

jira-pull-request-webhook bot commented Aug 13, 2024

ICU-22843 UnicodeString <-> std::u16string_view / wstring_view #3076

ICU-22843 UnicodeString <-> std::u16string_view / wstring_view #3076

Conversation

markusicu commented Aug 1, 2024 • edited Loading

Checklist

markusicu left a comment

Choose a reason for hiding this comment

markusicu Aug 2, 2024

Choose a reason for hiding this comment

richgillam left a comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Aug 3, 2024

aheninger commented Aug 5, 2024

richgillam left a comment

Choose a reason for hiding this comment

markusicu commented Aug 9, 2024

markusicu commented Aug 9, 2024

eggrobin commented Aug 9, 2024

eggrobin Aug 9, 2024

Choose a reason for hiding this comment

markusicu Aug 12, 2024

Choose a reason for hiding this comment

markusicu commented Aug 12, 2024

markusicu commented Aug 12, 2024

richgillam left a comment

Choose a reason for hiding this comment

richgillam commented Aug 12, 2024

jira-pull-request-webhook bot commented Aug 12, 2024

jira-pull-request-webhook bot commented Aug 13, 2024

markusicu commented Aug 1, 2024 •

edited

Loading