%S %C vfprintf contribution

[J. Johnston]

Artem B. Bityuckiy wrote:
> Hello Jeff!
> 
> J. Johnston wrote:
> 
>> Artem,
>>
>>   A number of comments/issues:
>>
>>   1. The tests for buf size vs MB_LEN_MAX shouldn't be needed.
>>      The lowest value is 40 and I don't think I want to see a character
>>      set that requires over 40 bytes to encode a single character. :)
> 
> 
> I don't think it is an error - it is just paranoid check. Theoretically, 
> MB_LEN_MAX may be > 40 (I think only about variable rang - not about 
> real multibyte characters). This is preprocessor check an it doesn't 
> make much overhead. :-) Please, remove this if you consider necessary.
>

It messes up the readibility of the code for no gain so I prefer to leave it out.

>> 2. Your 'S' code ends up falling into the old strlen check for 's'.  You
>>      should prevent this by using an else clause.
> 
> 
> Hmm, I think you missed about 'break' at line 728...
>

Yes I did.  It would be much clearer if it was done with if/else logic.  At the 
very least a comment after the widechar code is in order that explains "if you 
are here then the following xxxx is true".

>>   Your check for cp is
>>      NULL should occur at the top just after setting cp.  Your S code can
>>      then be an else if and the old 's' code can be in an else clause.
> 
> 
> If cp is NULL we consider this as if it was called with %s and set it to 
> "(null)". (This means, treat calls vfprintf("%S", NULL) = vfprintf("%s", 
> NULL)). In both cases "(null)" string will appead in file stream. Ther 
> is a check for cp != NULL at line 678. Is this an error?
> 

Nope, it was just to make things clearer.  In either case for a widechar or
non-widechar string, you have to check if the ptr is null and print "(null)".
You have added a large section of code specific to widechar in-between that has 
a null-check.  It is much clearer if you simply do the null case up front and 
then you don't have to check for null twice.

>>
>>   3. Your freeing of the widechar string buffer is not in the right spot.
>>      There is an error case that follows allocating the buffer so you 
>> won't
>>      free it.  
> 
> 
> Yes, you are right, I've missed this.
> 
>> You should also use a separate ptr other than cp for the
>>      malloc'd storage so there is no doubt you are freeing the right 
>> thing
>>      at the end.
> 
> 
> We are setting WIDECHARSTR flag only on line 719 after malloc. I think 
> it is no need to introduce another pointer.
> 

Actually, I would still prefer a separate pointer.  It prevents future changes 
from screwing up the free (e.g. somebody sets the flag too early on or in a 
different spot and cp has been incremented).  It also makes it easy to add other 
malloc'd storage as needed to other format specifiers and have a single free 
call that doesn't have to check for a specific flag.  The change I am asking for 
is trivial to do.

>>
>> -- Jeff J.
>>
>> Artem B. Bityuckiy wrote:
>>
>>> Artem B. Bityuckiy wrote:
>>>
>>>> Hello.
>>>>
>>>> Nicholas Wourms wrote:
>>>> > Artem B. Bityuckiy wrote:
>>>> >
>>>> >> Hello.
>>>> >>
>>>> >> Here is the patch that makes vfprintf (and hence, all other
>>>> >> vfprintf-based Xprintf functions) understand ISO C 90 %S (same as 
>>>> %ls)
>>>> >> and %C (same as %lc) format placeholders. Please, review it and give
>>>> >> us know if something is wrong.
>>>> >>
>>>> >> This is tested a little bit- it works and it seems should work with
>>>> >> any locale if wcrtomb and wcsrtomb functions work.
>>>> >>
>>>> >
>>>> > I believe the preferred patch type is unidiff or `diff -up`.
>>>> OK, here is the patch to vfprintf.c produced by diff -up.
>>>> >
>>>> > Cheers,
>>>> > Nicholas
>>>> >
>>>> Initially, we've added these changes to Newlib-1.11.0. They was 
>>>> tested with Newlib-1.11.0 too. But then I've copy changes to Newlib 
>>>> from CVS. vfprintf.c from CVS differs from Newlib-1.11.0's 
>>>> vfprintf.c. I didn't test (even compile) Newlib from CVS with these 
>>>> patch, but it must work.
>>>>
>>>> Thanks.
>>>>
>>>
>>
>>
> I've attached new diff -uN file.
>