Skip to content

Conversation

@lemire
Copy link
Member

@lemire lemire commented Jan 12, 2026

See #911

@lemire lemire marked this pull request as ready for review January 12, 2026 19:15
char output[2];
size_t written = simdutf::convert_utf16_to_utf8_safe(
input, 2, output, 2);
ASSERT_EQUAL(written, 2);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this verify written<=2 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pauldreik Hmmm... The first character in the input is é which should be converted to TWO bytes in UTF-8. No?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I was thinking we are just verifying the promise of not writing past the output length.

Copy link
Member Author

@lemire lemire Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind. Changing it.

@pauldreik
Copy link
Collaborator

I made a constexpr test for this, which also catches it:

namespace {
template <auto input, std::size_t buflen>
constexpr auto convert_insufficient_buf() {
  using namespace simdutf::tests::helpers;
  constexpr auto Noutput = simdutf::utf8_length_from_utf16(input);
  CTString<char8_t, buflen> output{};
  const auto ret = simdutf::convert_utf16_to_utf8_safe(input, output);
  if (ret == 0) {
    throw "failed conversion";
  }
  return output;
}
} // namespace
TEST(compile_time_check_of_issue_911) {
  using namespace simdutf::tests::helpers;
  constexpr auto input = u"\u00E9A"_utf16;
  constexpr auto expected = u8"\u00E9A"_utf8;
  constexpr auto actual = convert_insufficient_buf<input, 2>();
  constexpr auto N = std::min(actual.size(), expected.size());
  static_assert(expected.shrink<N>() == actual.shrink<N>());
}

@lemire
Copy link
Member Author

lemire commented Jan 12, 2026

I made a constexpr test for this, which also catches it:

Adding it.

@lemire lemire requested a review from pauldreik January 12, 2026 19:50
@lemire
Copy link
Member Author

lemire commented Jan 12, 2026

@pauldreik Have another look.

@lemire lemire merged commit 9d832c1 into master Jan 12, 2026
21 of 107 checks passed
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