[libc-commits] [PATCH] D74021: Created uChar implementation for libc

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Couple of high level comments:

1. Looks like the public `uchar.h` header or a rule to generate it are missing. Prefer generation. See `string.h` for example.
2. To define `char16_t` and `char32_t` types, use the predefined macro `__UINT_LEAST16__TYPE__` and `__UINT_LEAST32_TYPE__`. It would have been ideal if the free standing `stdint.h` supports `__need_*` macros, but it does not.

In D74021#1861627 <https://reviews.llvm.org/D74021#1861627>, @MarcusJohnson91 wrote:

> As for tests, I'm not sure where to begin with that, can anyone point me in the right direction?


Can you write unit tests using manually sythensized byte sequences.



================
Comment at: libc/src/uchar/c16rtomb.cpp:16
+
+size_t LLVM_LIBC_ENTRYPOINT(c16rtomb)(char *restrict s, char16_t c16,
+                                      mbstate_t *restrict ps) {
----------------
It would be good if you can add comments explaining the different cases that being handled here.


================
Comment at: libc/src/uchar/c16rtomb.cpp:25
+    StringSize = 1;
+    s = new char(StringSize);
+    s[0] = c16 & 0x7F;
----------------
abrachet wrote:
> For what its worth, this allocates one char and initializes it to StringSize. If you wanted to allocate StringSize chars you need `new char[StringSize]`. But `new` isn't safe to use for us, and I think it wouldn't even link because we have `-nostdlib` specified.
> 
> 
> ```lang=C
> void changeParam(int a) {
>     a = 5;
> }
> 
> int main() {
>    int a = 0;
>    changeParam(a);
>    assert(!a); // passes, a not changed.
> }
> ```
> The same is true with pointers if I pass my `char *` to c16rtomb, if you reassign to it by malloc or new, it changes the local variable but not the callers variable. These functions (unless passed a nullptr) assume that `s` is already pointing to enough valid memory.
If I am reading the standard correct, one should not need to allocate memory at all. It should be assumed that `s` is pointing to an appropriately sized byte array. So, one should not need `new`/`malloc` and any other allocator. I also think the same holds for the other functions as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74021/new/

https://reviews.llvm.org/D74021