Pali Rohár <[email protected]> wrote:
> This WideCharToMultiByte() function and its API is insane. I did not
> though about these new issues. Now I must admit that this code is too
> complicated.
>
> I was thinking how to simplify it, I wanted to write few ideas, but I
> have figured out that nothing worked which I tried due to insane API.
I wouldn't call it insane. Both `WideCharToMultiByte` and `MultiByteToWideChar`
are vere powerful functions which provide different ways to convert strings.
For example, unlike standard C conversion functions, they allow converting
input containing invalid multibyte characters or characters which cannot be
converted to target code page; in both cases they will be replaced with '?'.
I mixed up its behavior in my original changes, which is completely on me; I
should have looked up the documentation. I think them not allowing truncation
is actually a good thing.
> Below are just few small comments.
>
> I see that there is mix of style, sometimes there is space between
> function name and '('. And sometimes is not there. Would be nice to have
> style consistent across whole file.
I think it is only the call to `_assert` that has no space before '('. I don't
think it is a big deal.
> I think that this one change "crt: improve _wassert() emulation" is fine
> and could be merged.
>
> On Monday 22 June 2026 10:13:30 Kirill Makurin wrote:
>> Subject: [PATCH 3/4] crt: improve _wassert() emulation
>>
>> ...
>>
>> #include <assert.h>
>> +#include <limits.h>
>> +#include <stdbool.h>
>
> nit: As you are already including windows.h you can use BOOL type and
> then no need to include this stdbool.h (one inline file less).
I usually prefer not mixing windows.h types and standard types. This just my
preference.
>> + if (mbLength == 0) {
>
> I would really suggest to check as mbLength <= 0.
> Every such custom string conversion function can be a ticking bomb, so
> checking that returned values are valid should be always used.
I think checking return value from `WideCharToMultiByte`/`MultiByteToWideChar`
for being negative is paranoid. Microsoft documentation clearly states that
they return zero on failure. If either of them ever returns negative value,
then they do not conform to their specification and there is a bug.
I will agree with doing this if there is any Windows version which happened to
return negative values on failure.
>> + /**
>> + * Append "..." to the end of `buffer`.
>> + */
>> + memcpy (buffer, (const char[]) {'.', '.', '.', '\0'}, 4);
>
> This is very unusual way for specifying const string. Why not just
> classic way with? memcpy(buffer, "...", 4);
I'm not sure what I was thinking. Either way works though.
>> + if (message_length == 0) {
>> + message_length = 1;
>> + } else if (message_length > BUFSIZ) {
>> + truncate_message = true;
>> + message_length = BUFSIZ;
>> }
>
> There is missing case if the message_length is negative. I would suggest
> to use "if (message_length <= 0) {" to handle this edge case.
Addressed earlier.
- Kirill Makurin
_______________________________________________
Mingw-w64-public mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
