On 19.02.2018 18:17, Collin L. Walling wrote:
> On 02/19/2018 11:19 AM, Collin L. Walling wrote:
>> On 02/19/2018 11:00 AM, Thomas Huth wrote:
>>> On 19.02.2018 16:40, Collin L. Walling wrote:
>>>> On 02/17/2018 02:48 AM, Thomas Huth wrote:
>>>>> On 16.02.2018 23:07, Collin L. Walling wrote:
>>>>> [...]
>>>>>> +/**
>>>>>> + * uitoa:
>>>>>> + * @num: an integer (base 10) to be converted.
>>>>>> + * @str: a pointer to a string to store the conversion.
>>>>>> + * @len: the length of the passed string.
>>>>>> + *
>>>>>> + * Given an integer @num, convert it to a string. The string @str
>>>>>> must be
>>>>>> + * allocated beforehand. The resulting string will be null
>>>>>> terminated and
>>>>>> + * returned. This function only handles numbers between 0 and
>>>>>> UINT64_MAX
>>>>>> + * inclusive.
>>>>>> + *
>>>>>> + * Returns: the string @str of the converted integer @num
>>>>>> + */
>>>>>> +char *uitoa(uint64_t num, char *str, size_t len)
>>>>>> +{
>>>>>> + size_t num_idx = 0;
>>>>>> + uint64_t tmp = num;
>>>>>> +
>>>>>> + IPL_assert(str != NULL, "uitoa: no space allocated to store
>>>>>> string");
>>>>>> +
>>>>>> + /* Get index to ones place */
>>>>>> + while ((tmp /= 10) != 0) {
>>>>>> + num_idx++;
>>>>>> + }
>>>>>> +
>>>>>> + /* Check if we have enough space for num and null */
>>>>>> + IPL_assert(len > num_idx, "uitoa: array too small for
>>>>>> conversion");
>>>>> Well, in v5 of this patch you've had "len >= num_idx + 1" where we
>>>>> agreed that it was wrong. Now you have "len > num_idx" which is pretty
>>>>> much the same. WTF?
>>>>> I still think you need "len > num_idx + 1" here to properly take the
>>>>> trailing NUL-byte into account properly. Please fix it!
>>>>>
>>>>> Thomas
>>>>>
>>>> You're correct, and my apologies for not correcting the true problem
>>>> here:
>>>> I messed up the value of num_idx. It is off by one, but
>>>> initializing the
>>>> value to 1 instead of 0 should fix this. I must've accounted for
>>>> this in
>>>> my test file but forgot to update it in the actual source code.
>>> Are you sure that initializing it to 1 is right? Unless you also change
>>> the final while loop in this function, this will put the character into
>>> the wrong location ("str[num_idx] = num % 10 + '0';"). Imagine a number
>>> that only consists of one digit ... the digit will be placed in str[1]
>>> which sounds wrong to me...?
>>>
>>> Thomas
>>>
>> There's that, which we can solve by decrementing num_idx at the start
>> of the loop.
>> We also have to change the line str[num_idx + 1] = '\0'; to no longer
>> add 1.
>> It all boils down to "which way reads better", which I often struggle and
>> bounce back-and-forth with (way) too much...
>>
>> Maybe I should also rename num_idx to just "idx" and let the comments
>> explain
>> everything?
>>
> How is this for a compromise?
>
> - start num_idx at 1, provide comment as for why
> - change while loop comment to explain we are "counting the
> _indices_ _of_ _num_"
> - str[num_idx] is assigned \0, and we also decrement num_idx in one
> line
> - in conversion loop, post decrement num_idx as it is used
>
> char *uitoa(int num, char *str, int len)
> {
> int num_idx = 1; /* account for NULL */
> int tmp = num;
>
> assert(str != NULL, "uitoa: no space allocated to store string");
>
> /* Count indices of num */
> while ((tmp /= 10) != 0)
> num_idx++;
>
> /* Check if we have enough space for num and null */
> assert(len > num_idx, "uitoa: array too small for conversion");
>
> str[num_idx--] = '\0';
>
> /* Convert int to string */
> while (num_idx >= 0) {
> str[num_idx--] = num % 10 + '0';
> num /= 10;
> }
>
> return str;
> }
Yes, looks fine to me that way (with the "NUL" change mentioned by Eric).
Thomas
