Re: [Minios-devel] [UNIKRAFT PATCH 4/4] lib/nolibc: adapt sscanf code for Unikraft

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

Florian Schmidt <florian@xxxxxxxxx> writes:

> On 07/27/2018 05:29 PM, Yuri Volchkov wrote:
>> 1) Use the right includes
>> 2) (u_)quad_t => (u)int64_t
>> 3) u_char => unsigned char
>> 4) strto(u)q => strto(u)ll
>> 5) bcopy => memmove
>> 6) fix warnings generated by modern gcc (8.1.1)
>
> That's the explicit casts to ccfntype and int that you added?
Is it a question regarding Nr 6? Then the answer is inline. Otherwise,
could you clarify it a bit, please?

>
>> diff --git a/lib/nolibc/include/stdio.h b/lib/nolibc/include/stdio.h
>> index 073b132..6d5652f 100644
>> --- a/lib/nolibc/include/stdio.h
>> +++ b/lib/nolibc/include/stdio.h
>> @@ -64,6 +64,9 @@ int   fflush(FILE *fp);
>>   int vprintf(const char *fmt, va_list ap);
>>   int  printf(const char *fmt, ...)                           __printf(1, 2);
>>   
>> +int vsscanf(const char *str, const char *fmt, va_list ap);
>> +int  sscanf(const char *str, const char *fmt, ...) __scanf(2, 3);
>
> I would align the __scanf with the preceding __printf statement for 
> consistency.
Agree

>
>> @@ -271,32 +261,32 @@ literal:
>>               */
>>              case 'd':
>>                      c = CT_INT;
>> -                    ccfn = (ccfntype)strtoq;
>> +                    ccfn = (ccfntype)strtoll;
>>                      base = 10;
>>                      break;
>>   
>>              case 'i':
>>                      c = CT_INT;
>> -                    ccfn = (ccfntype)strtoq;
>> +                    ccfn = (ccfntype)strtoll;
>>                      base = 0;
>>                      break;
>>   
>>              case 'o':
>>                      c = CT_INT;
>> -                    ccfn = strtouq;
>> +                    ccfn = (ccfntype) strtoull;
>>                      base = 8;
>>                      break;
>
> Whenever you added your explicit cast, you put a space there; when you 
> only changed the function name, you left it without a space. Could you 
> make this consistent? (I personally prefer the version without a space 
> for casts, but I don't think we have a coding style rule for it. Just 
> consistent inside the same switch statement would be nice.)
Agree.

It just slipped from my attention, because I did automatic replacing. So
if has nothing to do with function. It has to do with code used to be
there before.

>
> This applies to the ones, below, too:
>
>>   
>>              case 'u':
>>                      c = CT_INT;
>> -                    ccfn = strtouq;
>> +                    ccfn = (ccfntype) strtoull;
>>                      base = 10;
>>                      break;
>>   
>>              case 'x':
>>                      flags |= PFXOK; /* enable 0x prefixing */
>>                      c = CT_INT;
>> -                    ccfn = strtouq;
>> +                    ccfn = (ccfntype) strtoull;
>>                      base = 16;
>>                      break;
>>   
>> @@ -318,7 +308,7 @@ literal:
>>              case 'p':       /* pointer format is like hex */
>>                      flags |= POINTER | PFXOK;
>>                      c = CT_INT;
>> -                    ccfn = strtouq;
>> +                    ccfn = (ccfntype) strtoull;
>>                      base = 16;
>>                      break;
>>   
>
>
>
>> diff --git a/lib/nolibc/stdio.c b/lib/nolibc/stdio.c
>> index 7e3d368..3a32907 100644
>> --- a/lib/nolibc/stdio.c
>> +++ b/lib/nolibc/stdio.c
>> @@ -289,6 +289,7 @@ reswitch:
>>                      goto handle_nosign;
>>              case 'X':
>>                      upper = 1;
>> +                    /* Fall through */
>>              case 'x':
>>                      base = 16;
>>                      goto handle_nosign;
>> 
>
> That last bit is unrelated to the rest of the patch?
Oh.. That was me not careful enough. Will remove it. And this was
actually the number 6 ("fix warnings generated by modern gcc") from the
commit message, if you question was about that.

It probably would not hurt to send a separate patch (outside of this
series), since I already hit this thing.

>
>
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/minios-devel

-- 
Yuri Volchkov
Software Specialist

NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel