On 23.01.2018 23:33, Collin L. Walling wrote:
> On 01/23/2018 02:23 PM, Eric Blake wrote:
>> On 01/23/2018 12:26 PM, Collin L. Walling wrote:
>>> [...]
>>> +/**
>>> + * atoi:
>>> + * @str: the string to be converted.
>>> + *
>>> + * Given a string @str, convert it to an integer. Leading whitespace is
>>> + * ignored. The first character (after any whitespace) is checked
>>> for the
>>> + * negative sign. Any other non-numerical value will terminate the
>>> + * conversion.
>>> + *
>>> + * Returns: an integer converted from the string @str.
>>> + */
>>> +int atoi(const char *str)
>>> +{
>>> + int val = 0;
>>> + int sign = 1;
>>> +
>>> + if (!str || !str[0]) {
>>> + return 0;
>>> + }
>>> +
>>> + while (*str == ' ') {
>>> + str++;
>>> + }
>> That's not "any whitespace", but only spaces. A fully compliant
>> implementation would be checking isspace(), but I don't expect you to
>> implement that; at a minimum, also checking '\t' would get you closer
>> (but not all the way to) compliance.
>
>
> I'll fix the comment to be more clear.
>
> I think it's okay to just have the menu code treat any other kind
> of whitespace as an error (it will check before calling atoi). I
> added support for negatives in bothfunctions because it was easy
> enough to do so and for the sakeof completeness.
>
> However, I worry trying to be 100% compliant will just bloat the
> code when we only need it for very specific use cases.
>
> Would you say what we have (along with the fix to itostr below) is
> sufficient enough?
IMHO the current way is good enough for a BIOS implementation. We're not
doing a full replacement of glibc here ;-)
>
>>
>>
>>> +static char *_itostr(int num, char *str, size_t len)
>>> +{
>>> + int num_idx = 0;
>>> + int tmp = num;
>>> + char sign = 0;
>>> +
>>> + if (!str) {
>>> + return NULL;
>>> + }
>>> +
>>> + /* Get index to ones place */
>>> + while ((tmp /= 10) != 0) {
>>> + num_idx++;
>>> + }
>>> +
>>> + if (num < 0) {
>>> + num *= -1;
>>> + sign = 1;
>>> + }
>> If num == INT_MIN, then num is still negative at this point...
>>
>>> +
>>> + /* Check if we have enough space for num, sign, and null */
>>> + if (len <= num_idx + sign + 1) {
>>> + return NULL;
>>> + }
>>> +
>>> + str[num_idx + sign + 1] = '\0';
>>> +
>>> + /* Convert int to string */
>>> + while (num_idx >= 0) {
>>> + str[num_idx + sign] = num % 10 + '0';
>> ...which breaks this.
>>
>> Either make it work, or document the corner case as unsupported.
>>
>
> Might as well just make it work at this point:
>
> #define INT32_MIN 0x80000000
>
> static char *itostr(int num, char *str, size_t len)
> {
> int num_idx = 0;
> int tmp = num;
> char sign = !!(num & INT32_MIN);
>
> if (!str) {
> return NULL;
> }
>
> /* Get index to ones place */
> while ((tmp /= 10) != 0) {
> num_idx++;
> }
>
> /* Check if we have enough space for num, sign, and null */
> if (len <= num_idx + sign + 1) {
> return NULL;
> }
>
> str[num_idx + sign + 1] = '\0';
>
> if (sign) {
> str[0] = '-';
> if (num == INT32_MIN) {
> str[num_idx + sign] = '8';
> num /= 10;
> num_idx--;
> }
> num *= -1;
> }
>
> /* Convert int to string */
> while (num_idx >= 0) {
> str[num_idx + sign] = num % 10 + '0';
> num /= 10;
> num_idx--;
> }
>
> return str;
> }
>
> Thoughts?
Looks fine to me. With that modification:
Reviewed-by: Thomas Huth <th...@redhat.com>
