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

[Pedro Alves <palves@redhat.com>]

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