[libc-commits] [libc] [libc] Add UTF-16 support in CharacterConverter [1/6] (PR #230434)

[Michael Jones via libc-commits]

================
@@ -127,6 +194,57 @@ LIBC_INLINE int CharacterConverter::push(char8_t utf8_byte) {
   return EILSEQ;
 }
 
+LIBC_INLINE int CharacterConverter::push(char16_t utf16) {
+  // Checking for high surrogate if first push
+  if (isEmpty()) {
+    bool is_surrogate = (utf16 & 0xF800) == 0xD800;
+    if (is_surrogate) {
+      bool is_high_surrogate = (utf16 & 0x400) == 0;
+      if (!is_high_surrogate) {
+        // unpaired surrogate -> reset the state
+        // bytes_stored and total_bytes will always be 0 here
+        state->partial = static_cast<char32_t>(0);
+        return EILSEQ;
+      }
----------------
michaelrj-google wrote:

code with nested conditions like this can be hard to read. Nesting three `if`s is right about the limit of what I'd consider reasonable, and if possible I'd recommend keeping it lower. Nesting makes it harder to understand exactly what paths can lead to a certain result, for example in this case `is_high_surrogate` only checks if the `0x400` is set in the input value, since `is_surrogate` already checked the other high bits. One way to simplify is to figure out which conditions are actually independent, and which ones return early. In this case, if the conversion state is empty and `utf16` is a high surrogate, that means it's an invalid encoding and we can return `EILSEQ` immediately.

I recommend moving the `is_high_surrogate` check above the `is_surrogate` check, like this:
```suggestion
  if (isEmpty()) {
    bool is_high_surrogate = (utf16 & <new value>) == 0;
    if (!is_high_surrogate) {
      // unpaired surrogate -> reset the state
      // bytes_stored and total_bytes will always be 0 here
      state->partial = static_cast<char32_t>(0);
      return EILSEQ;
    }
    bool is_surrogate = (utf16 & 0xF800) == 0xD800;
    if (is_surrogate) {
```

https://github.com/llvm/llvm-project/pull/230434