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

[Marcus Johnson via Phabricator via libc-commits]

MarcusJohnson91 added a comment.

In D74021#1936199 <https://reviews.llvm.org/D74021#1936199>, @sivachandra wrote:

>   Are the functions `char[16|32]rtomb` doing a UTF-16|32 to UTF-8 conversion? Per the standard, they should convert to the current locale?


Which standard are you referring to? I'm reading the C18 (N2176) standard just to be sure we're on the same page.

As for the c16rtomb and c32rtomb functions, they're defined in the uchar.h header, and the C18 standard says "7.28 Unicode utilities <uchar.h>", I take that to mean that these functions should convert between different representations of Unicode.

> 2. You mention building multi-byte support over wide char support. However, if I am reading it right, it doesn't seem like it?

I not sure what you're referring to here? Do you mean where I said that I only intend to implement enough of the wchar header to support uchar, aka declare the mbstate_t type, and the mbstate_init function; and only becaose the standard declares this type and function in wchar, maybe it'd be better to put these parts in an internal uchar implementation file?

> A generic comment: I think you are not using pointers correctly. For example, in the `mbrtoc16` function, you have this:
> 
>   if ((s & 0x80) == 0) {
>       // ASCII
>   } else if ((s & 0x80) == ) 
>   ...
>    
> 
> `s` is of type `const char *restrict`. Seems to me like that the intention here is to compare first the character `*s`?

Yes, you're right I originally wrote the code using different variable names using a very different implementation, and I messed up the syntax when copypasting the correct variable names and overlooked them when changing them.

> In few other places, I see incomplete code. Is the patch ready for review?

Not quite, there's still a few things I'm working on, and I'm working on a few different patches at the same time as well.


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

https://reviews.llvm.org/D74021