ICU-23271 Fix a possible error about ambiguous overloads of U_ICU_NAMESPACE::internal::toU16StringView

[matthew-wozniczka]

See https://unicode-org.atlassian.net/browse/ICU-23271 & matthew-wozniczka/icu-23271-reproducer@9c8cf8a (Note: For the latter, use the HEAD of the main branch to test)

When using ICU (noticed in the 77.1 release, didn't seem to be an issue in 74.2) in a configuration where the encoding of wchar_t is UTF-16 (noticed on 32-bit AIX & Windows), using the += operator on a subclass of UnicodeString could give a compilation error about an ambiguous reference to U_ICU_NAMESPACE::internal::toU16StringView

I fixed it (at least for our use-case) by modifying the overload meant to handle std::u16string_view itself to also handle any types which are implicitly convertible to it (as UnicodeString is)

Checklist

• Required: Issue filed: ICU-23271
• Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
• Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
• Issue accepted (done by Technical Committee after discussion)
• Tests included, if applicable
• API docs and/or User Guide docs changed or added, if applicable
• Approver: Feel free to merge on my behalf

[CLAassistant]

All committers have signed the CLA.

[markusicu]

• Please try to add a test case, ideally one that would fail on one of our CI platforms (e.g., Windows).
• The DirectlyConvertibleToU16StringView looks like just an alias; I think we want to keep using the std::is_convertible_v thing.
• Why remove inline?
• I am asking @eggrobin to take a close look.

[matthew-wozniczka]

Please try to add a test case, ideally one that would fail on one of our CI platforms (e.g., Windows).

I have a reproducer (linked in OP), not sure how to fit it into your tests, any guidance? Also, it won't fail after the change, so I'm not sure exactly how it should work?

The DirectlyConvertibleToU16StringView looks like just an alias; I think we want to keep using the std::is_convertible_v thing.

I saw ConvertibleToU16StringView below & thought it appropriate, but it's easy to change. (Not using it multiple times anymore so it makes less sense as well)

Why remove inline?

Templates are implicitly inline, so it's redundant, but it can be put back if ICU's coding style demands it.

[eggrobin]

You mention that the issue occurs when (emphasis mine)

using the += operator on a subclass of UnicodeString

The documentation explicitly says

icu/icu4c/source/common/unicode/unistr.h

Line 232 in 9f1c574

* The UnicodeString class is not suitable for subclassing.

So I think we would want to see a reproducible issue without subclassing UnicodeString before we start playing around with this overload set.

[eggrobin]

(Incidentally, since this has been possible since C++11, we might want to make UnicodeString final, instead of saying that you should not subclass it in the middle of a hundred-line comment.)

EDIT: filed ICU-23284.

[matthew-wozniczka]

You mention that the issue occurs when (emphasis mine)

using the += operator on a subclass of UnicodeString

The documentation explicitly says

icu/icu4c/source/common/unicode/unistr.h

Line 232 in 9f1c574

* The UnicodeString class is not suitable for subclassing.

So I think we would want to see a reproducible issue without subclassing UnicodeString before we start playing around with this overload set.

Ahh, if the official word is that subclassing is not allowed, I guess we'll have to change our implementation strategy...

A bit problematic since I was relying on this being possible to accomplish some noexcept-ness (Have one simba_wstring class which has unique ownership of the simba_wstring_impl, and another SharedWString class which has shared ownership of one, and we currently support a SharedWString::SharedWString(simba_wstring&&) noexcept), not sure if I still can, but I guess that's my problem (though suggestions are welcome!).