From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Garzik Subject: Re: [PATCH hail] const-correctness tweaks Date: Wed, 20 Oct 2010 04:36:45 -0400 Message-ID: <4CBEAA1D.2030302@garzik.org> References: <87mxqro78a.fsf@meyering.net> <4CBE1AA2.5020004@garzik.org> <878w1t5m5k.fsf@meyering.net> Mime-Version: 1.0 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:sender:message-id:date:from :user-agent:mime-version:to:cc:subject:references:in-reply-to :content-type:content-transfer-encoding; bh=xS/nuKGiT23lfBX3GYxgC57yKZmVFzlIGOS6WhvLR+U=; b=VVEKsswsJq1eqq6ebuPAMZ2HITi4tQLyNhsUVbB4AM0NF44wBOI6kQpwzJbH1zvlvW M6E2LJ9AHhJC4z0ot1CgDUsOEtNI0xAq5gGHC6HvT30dAfUvLfw0bYFueydtC/0GVeBL BhX6yrT3k9FC99x1fGej7ehiabZkyA6SwCJ8s= In-Reply-To: <878w1t5m5k.fsf@meyering.net> Sender: hail-devel-owner@vger.kernel.org List-ID: Content-Type: text/plain; charset="utf-8"; format="flowed" To: Jim Meyering Cc: Project Hail On 10/20/2010 04:00 AM, Jim Meyering wrote: > Jeff Garzik wrote: > >> On 10/06/2010 08:07 AM, Jim Meyering wrote: >>> >>> Make write_cb callback's buffer parameter const, like all write-lik= e >>> functions. >>> Give a few "char *" parameters the "const" attribute. >>> >>> Signed-off-by: Jim Meyering >>> --- >>> >>> It looks like most of hail's interfaces are const-correct, >>> but one stood out because it provokes a warning when I tried to >>> pass a const-correct write_cb function to hstor_get from iwhd: >>> >>> proxy.c:382: warning: passing argument 4 of 'hstor_get' from = \ >>> incompatible pointer type >>> /usr/include/hstor.h:173: note: expected \ >>> 'size_t (*)(void *, size_t, size_t, void *)' but argument = is of type \ >>> 'size_t (*)(const void *, size_t, size_t, void *)' >>> >>> In case you feel comfortable fixing this, here's a patch: >> >> >> Sorry for not getting back to this; I had hoped to solve some >> additional problems that cropped up, but didn't have time. So, to >> forestall further delay, >> >> libtool: compile: gcc -DHAVE_CONFIG_H -I. -I.. -I../include -pthrea= d >> -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include >> -I/usr/include/libxml2 -O2 -Wall -Wshadow -g -MT hutil.lo -MD -MP -M= =46 >> .deps/hutil.Tpo -c hutil.c -o hutil.o >> hutil.c: In function =E2=80=98hreq_hdr_push=E2=80=99: >> hutil.c:145: warning: assignment discards qualifiers from pointer >> target type >> hutil.c:146: warning: assignment discards qualifiers from pointer >> target type >> >> warnings appear after this patch. When solving these warnings with >> const' markers, it quickly becomes a bit of a rat's nest. >> >> At a minimum, the write_cb callback signature must match libcurl's, >> which does not use 'const'. I can see this makes sense from libcurl >> implementation's perspective, even if it does not really match the >> constness one expects from a foo-get function. > > Hi Jeff. > > Sorry I didn't notice that the first time. > I built with ./autogen.sh&& ./configure&& make. > It looks like you recommend -Wall -Wshadow. > > The two warnings above are the only ones I see with the patch, > and they're easy to fix. When storing const pointer params into > a struct like that, I've found that it's best to cast away the "const= ", > which really does reflect the semantics: by using "const" on the > parameter, I view it as promising not to deref through the pointer > *in that function*. Since it's usually not reasonable to make > the struct member "const" (as you saw, it propagates too far > and often ends up being contradictory), the lesser evil of the cast > is preferable here. > > If you're still game, the following incremental patch seems to be > enough for me: Let me know and I'll resubmit the full one. Well, my primary concern now originates from curl_easy_setopt(3)=20 documentation: CURLOPT_WRITEFUNCTION Function pointer that should match the following prototype: size_t function( void *ptr, size_t size, size_t nmemb, void *stream); hstor's callback is passed directly to libcurl, so we seem to be bound=20 by outside constraints, no? Jeff