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

[Alex Brachet via Phabricator via libc-commits]

abrachet added a comment.

These need tests.



================
Comment at: libc/src/uchar/CMakeLists.txt:7
+    mbrtoc16.h
+  #DEPENDS
+)
----------------
remove comments


================
Comment at: libc/src/uchar/CMakeLists.txt:36
+)
\ No newline at end of file

----------------
newline


================
Comment at: libc/src/uchar/c16rtomb.cpp:15
+    
+    size_t LLVM_LIBC_ENTRYPOINT(c16rtomb)(char * restrict s, char16_t c16, mbstate_t * restrict ps) {
+        size_t StringSize = 0ULL;
----------------
Too long I guess. clang-format


================
Comment at: libc/src/uchar/c16rtomb.cpp:16
+    size_t LLVM_LIBC_ENTRYPOINT(c16rtomb)(char * restrict s, char16_t c16, mbstate_t * restrict ps) {
+        size_t StringSize = 0ULL;
+        /*
----------------
Remove ULL.


================
Comment at: libc/src/uchar/c16rtomb.cpp:18-21
+         s = output string of UTF-8 code units
+         c16 = the non-surrogate pair UTF-16 code unit to convert to UTF-8
+         ps = string position?
+         return value = size of s in bytes aka the number of bytes that had to be added
----------------
We don't do block comments. Use //. Anyway I don't think you need to document library functions like this.


================
Comment at: libc/src/uchar/c16rtomb.cpp:29
+            StringSize = 1;
+            s          = calloc(StringSize, sizeof(uint8_t));
+            s[0]       = C16 & 0x7F;
----------------
This is not how pointers work. I'm not sure what the point of this is.


================
Comment at: libc/src/uchar/c16rtomb.h:16
+    
+    size_t c16rtomb(char * restrict s, char16_t c16, mbstate_t * restrict ps);
+    
----------------
No indentation. These all need clang-format run on them.


================
Comment at: libc/src/uchar/c32rtomb.cpp:19-20
+            StringSize = 1;
+            s          = calloc(StringSize, sizeof(uint8_t));
+            s[0]       = c32 & 0x7F;
+        } else if (c32 <= 0x7FF) {
----------------
We don't align like this. clang-format will fix these.


================
Comment at: libc/src/uchar/mbrtoc16.cpp:19-20
+        
+        switch (n) {
+            case 1:
+                Decoded                 =  s[0] & 0x7F;
----------------
I think case statements should be aligned with the switch. clang-format will tell us either way :)


================
Comment at: libc/src/uchar/mbrtoc32.cpp:17
+        size_t StringSize  = 1ULL;
+        char32_t Decoded   = calloc(1, sizeof(char32_t));
+        
----------------
What's the point of this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74021