Hi,
On 03/27/2017 05:48 AM, Supragya Raj wrote:
> This is the patch for implementation of ptrdiff_t in printf_core.c file.
> This implementation was done earlier, however the presentation did not
> adhere to the correct ways. This mail includes a bzr diff file that
> implements just that.
The patch looks much better now. I have a few comments:
- There is a comment right above your fix, which still says that this
feature is unimplemented. Shouldn't the patch remove this comment?
- For the sake of consistency, can you make the same changes also to the
printf_core.c in kernel/ and in boot/?
- The else branch here:
if (sizeof(ptrdiff_t) == sizeof(uint32_t))
qualifier = PrintfQualifierInt;
else if(sizeof(ptrdiff_t) == sizeof(uint64_t))
qualifier = PrintfQualifierLongLong;
else
goto next_char;
seems overzealous to me. The configuration of the system should not
allow it. ptrdiff_t simply is either 32-bit or 64-bit. So the test for
the third possibility is not warranted. Either drop it completely, or
use a static_assert()... I'd say drop it and make it a mere if-else test.
- Could you extend some of the print tests in app/tester to also test
%td and friends? This way it will be easy to see that it does what it is
supopsed to do.
> P.S. @jermer, in this patch, i have overcome the issue of additional
> whitespaces. Somehow ATOM induced them in the file earlier. So used vim
> to redo it. :-).
Good for you :-)
Thanks,
Jakub
_______________________________________________
HelenOS-devel mailing list
HelenOS-devel@lists.modry.cz
http://lists.modry.cz/listinfo/helenos-devel
