From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26190 invoked by alias); 5 Mar 2013 12:59:23 -0000 Received: (qmail 26181 invoked by uid 22791); 5 Mar 2013 12:59:22 -0000 X-SWARE-Spam-Status: No, hits=-8.0 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 05 Mar 2013 12:59:13 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r25CxBrF006109 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 5 Mar 2013 07:59:11 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r25Cx9mF025697; Tue, 5 Mar 2013 07:59:10 -0500 Message-ID: <5135EC1D.9030601@redhat.com> Date: Tue, 05 Mar 2013 12:59:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130219 Thunderbird/17.0.3 MIME-Version: 1.0 To: Yao Qi CC: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] use zuinteger_unlimited for some remote commands References: <1360934868-5807-1-git-send-email-yao@codesourcery.com> <1360934868-5807-2-git-send-email-yao@codesourcery.com> <51349F6E.8020101@redhat.com> <5134B192.8080507@codesourcery.com> <5134C9DD.2070205@redhat.com> <5135A26A.6030608@codesourcery.com> In-Reply-To: <5135A26A.6030608@codesourcery.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2013-03/txt/msg00144.txt.bz2 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 > > * 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