On Tue, 2018-01-16 at 11:27 -0500, Steven Rostedt wrote:
>
>
> :-) That was totally lost in translation. :-)
>
> No, I didn't mean to have a comment literally saying "why would strtol
> return zero and this not be an error", I meant for the comment to
> explain it.
>
> Actually, looking at the man page which states:
>
Yep, I got it.
Sometimes I interpret words too literally. My fault :-)
> I say we simply remove the comment. Or say what the man page example
> says:
>
> /* Check for various possible errors */
>
> and leave it at that.
Sure, "Check for various possible errors" sounds good to me.
>
> Sure it could be negative. The point was, you don't want it to be if
> you do:
>
> buf[0] = new_status + '0';
>
> As that will break if new_status is negative or greater than 9.
>
> Also, whether you use unsigned, or do the above, they both have the
> same result. A negative produces a warning. Which is fine. As long as
> it doesn't kill the program. It's only an implementation detail.
>
> That is, using unsigned char as new_status, and checking
>
> if (new_status > 9)
>
> Is no different than using int and checking
>
> if (new_status < 0 || new_status > 9)
>
> except that you use more instructions to accomplish the same thing.
>
Sure, using two checks with 'int' is less efficient then using the 'unsigned
trick',
but my point is that such a function (at interface level) should accept exactly
the same type 'returned' (via OUT param) by read_proc(). It should be symmetric,
as if instead of 'int/unsigned' we used an opaque type 'value_t' for which we
cannot
make assumptions. Clearly, the implementation may in practice accept a subset
of the values
allowed by the parameter type.
What about accepting 'int' but doing the check this way:
if ((unsigned)new_status > 9) {
warning(...);
return;
}
This way, we'll keep the interface symmetric (with read_proc()) but, at the
same time,
we use a more efficient check.
--
Vladislav Valtchev
VMware Open Source Technology Center
