Skip to content

Conversation

@philipp-zimmermann
Copy link

Changes

The rvalue overloads for icu::UnicodeString::tempSubString and icu::UnicodeString::tempSubStringBetween are deleted.

Justification

Calling tempSubString on a rvalue violates the precondition ("[...] requires that the original string not be modified or deleted during the lifetime of the returned substring object."). Therefore disallowing usage like this prevents bugs.

A example for the buggy behavior is the following code:

#include <iostream>
#include <unicode/unistr.h>

[[nodiscard]] auto get_string() -> icu::UnicodeString
{
  return icu::UnicodeString {"abcdefg"};
}

auto print (icu::UnicodeString const& unicode_string) -> void
{
  std::string s;
  unicode_string.toUTF8String (s);
  std::cout << s << '\n';
}

int main()
{
  {
    auto const substring {get_string().tempSubString(2)};

    print (substring);
    std::string s;
    substring.toUTF8String (s);
    std::cout << s << '\n';
    print (substring);
  }

  std::cout << "\nCorrect behavior:\n";

  {
    auto const full_string {get_string()};
    auto const substring {full_string.tempSubString(2)};

    print (substring);
    std::string s;
    substring.toUTF8String (s);
    std::cout << s << '\n';
    print (substring);
  }
}

This may lead to output like this:

cdefg
efg
晥gg

Correct behavior:
cdefg
cdefg
cdefg

The above output is from a binary compiled with "Release" mode. When using "Debug" the behavior is correct.
This is tested with https://github.com/unicode-org/icu/releases/download/release-78.2/icu4c-78.2-Ubuntu22.04-x64.tgz.

Jira issue

I am not able to create a new jira issue at all and I'm not able to find a solution for this problem.

TODO: Fill out the checklist below.

Checklist

  • Required: Issue filed: ICU-NNNNN
  • 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)
  • [x ] 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
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Philipp Zimmermann seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

2 participants