Re: [PATCH 1/2] use zuinteger_unlimited for some remote commands

Pedro Alves
2013-03-05T12:59:23Z

On 03/05/2013 07:44 AM, Yao Qi wrote:
> On 03/05/2013 12:20 AM, Pedro Alves wrote:
>> But I ask, what's either:
>>
>> - the point of making it internally signed
>> with things like:
>>
>> if (*(int *) c->var != val)
>>
>> and forbidding INT_MAX..UINT_MAX-1. (Not that I'm
>> arguing it should). The care to only accept -1
>> and not any other negative number made me think
>> numbers in that range should be accepted.
>
> Internally, we use -1 for unlimited, so it is signed. Externally, it
> is unsigned, so we have to forbid the range (INT_MAX, UINT_MAX - 1].

No, we don't _have_ to. We could as well make it internally
unsigned as well, and only reserve UINT_MAX, thus the range
of values would become [0, UINT_MAX-1], with UINT_MAX meaning
unlimited/"full throttle". That'd be practically double the
range of values the setting supports.
That'd make a lot more sense, if you wanted to justify making
the variable unsigned.

>> - the point of making it externally unsigned if
>> it only accepts [0, INT_MAX]. If the variable
>
> because we need -1 for unlimited internally.

You're just reading the code to me, not explaining it.

We don't _need_ -1 for unlimited internally. We could just as
well treat the variable internally as the correct type of
unsigned int, and handle UINT_MAX as unlimited internally too.
In fact, that'd be the _correct_ way to code it. Assuming
2's complement _representation_ (unlike signed->unsigned
conversion), as in

case var_zuinteger_unlimited:
{
if (*(int *) c->var == -1)

while the object at c->var is unsigned int, is not valid C.

There's really absolutely no reason at all for doing things
internally and externally differently.

> I don't see anything
> wrong to define unsigned type whose range is [0, INT_MAX].

>
>> assigned to the command was signed too, then this
>> range would be both implicit and explicit, meaning, one
>> weird detail less the user of the API needs to know.
>
> If I understand you correctly, this is what you want,
>
> extern void
> add_setshow_zuinteger_unlimited_cmd (char *name,
> enum command_class class,
> int *var,
> ^^^^^^^^
> then, IMO, the API is weird, in which the type of parameter VAR
> (singed) is not consistent with the function (zuinteger_unlimited).
> Since these CLI stuff provide APIs to other components, it is better to
> review them externally first.

In turn, the external API was weird in that it has a hole that
is easy to fall into -- the [INT_MAX..UINT_MAX-1] range. Signed
makes it dead simple -- "The whole set of positive values that fit
in 'int' is valid for regular uses; negative values are special", and
gives the compiler a chance to catch problems. Sign based clearly
signals to the API user (which includes uses of the variable beyond
the set/show commands themselves) that they can't go beyond INT_MAX.

>
> However, I don't insist on that if you believe changing the type of VAR
> from "unsigned int" to "int" is better, I am OK to change it to:
>
> /* ZeroableUnsignedInteger with unlimited value. *VAR is an
> int, but its range is [0, INT_MAX]. -1 stands for
> unlimited and other negative numbers are not allowed. */
>

Thanks, that's better.

I like that we no longer assume 2's complement in the API
description, talking about unsigned and -1 (while API clients
actually used UINT_MAX).

> 2013-03-05 Yao Qi <yao@codesourcery.com>
>
> * cli/cli-decode.c (add_setshow_zuinteger_unlimited_cmd): Change
> parameter VAR's type from "unsigned int" to "int".
> * command.h (var_zuinteger_unlimited): Update its comments.
> (add_setshow_zuinteger_unlimited_cmd): Update the declaration.

This is OK.

--
Pedro Alves