This is the mail archive of the newlib@sources.redhat.com mailing list for the newlib project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: %S %C vfprintf contribution


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.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]