From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/2812 Path: news.gmane.org!not-for-mail From: Isaac Dunham Newsgroups: gmane.linux.lib.musl.general Subject: Re: strverscmp Date: Mon, 18 Feb 2013 17:15:58 -0800 Message-ID: <20130218171558.bf161621.idunham@lavabit.com> References: <20130201071053.GA14593@brightrain.aerifal.cx> <20130201182340.c60061b6.idunham@lavabit.com> <20130202023833.GW20323@brightrain.aerifal.cx> <20130201200452.26687bed.idunham@lavabit.com> <20130217183944.GF20323@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="Multipart=_Mon__18_Feb_2013_17_15_58_-0800_C4T4O9M+90oUF7fC" X-Trace: ger.gmane.org 1361236578 15795 80.91.229.3 (19 Feb 2013 01:16:18 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 19 Feb 2013 01:16:18 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-2813-gllmg-musl=m.gmane.org@lists.openwall.com Tue Feb 19 02:16:40 2013 Return-path: Envelope-to: gllmg-musl@plane.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1U7bow-0008JP-5U for gllmg-musl@plane.gmane.org; Tue, 19 Feb 2013 02:16:34 +0100 Original-Received: (qmail 9217 invoked by uid 550); 19 Feb 2013 01:16:12 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 8181 invoked from network); 19 Feb 2013 01:16:12 -0000 DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=lavabit; d=lavabit.com; b=k6MbHqHx1xZws6tISxGN5av43GQu4QZi1RCm+ccQUI0p8oR+/uOJIGW8mjBZqBNbaiGB00+5Qwe3VcwyODfKPKuq4UiSPqGShWNrLNUEYnuxgsJTJNOCPmMzKqCqCkjv5S4O0xscQEVc0LhfUoSvYn66zEiLH+O9PYCb7q2lh7Y=; h=Date:From:To:Subject:Message-Id:In-Reply-To:References:X-Mailer:Mime-Version:Content-Type; In-Reply-To: <20130217183944.GF20323@brightrain.aerifal.cx> X-Mailer: Sylpheed 3.0.2 (GTK+ 2.20.1; i486-pc-linux-gnu) Xref: news.gmane.org gmane.linux.lib.musl.general:2812 Archived-At: This is a multi-part message in MIME format. --Multipart=_Mon__18_Feb_2013_17_15_58_-0800_C4T4O9M+90oUF7fC Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Sun, 17 Feb 2013 13:39:44 -0500 Rich Felker wrote: > > Hey, sorry it's taken me so long to get to reading and reviewing this > well. Here are some comments, mostly stylistic, but one's about a > corner-case bug. See below. With that said, the algorithm looks right > to me, and I think we can commit soon. > > Rich All the comments seem like improvements to me. Attached is a revised version. > > + while (*l && *r && l[0]==r[0]){ > > I used to do a lot of while loops like this, but now I feel like it > makes it hard to follow the end-of-string logic -- a lot more code > runs after the null terminator is hit, and it's nontrivial to follow > through it all and make sure the right thing happens, as the > subsequent code after the loop is now handling two very-distinct > loop-exit conditions: > > 1. Loop exited due to mismatch. > 2. Loop exited due to hitting end-of-string with no mismatch. > > What about instead doing something like this: > > while (*l==*r) { > if (!*l) return 0; > /* ... */ > > That way, the "equal strings" case is trivially correct. It also might > generate better code. OK. > Note that I also made a couple stylistic changes here; unless you > object, I prefer *p to p[0] when p is just being used as a moving > pointer within a string, always accessed with offset 0. It's just more > concise/less cluttered and avoids suggesting it's used as a string in > itself. Also added a space before the open brace, for consistency with > style elsewhere. > > + if ((isdigit(l[0]) && isdigit(r[0]) ) && haszero) { > > + int lenl=0, lenr=0, firstl=l[0], firstr=r[0]; > > These variables have the wrong types. int is not sufficient to store a > character count in a string. You need size_t. > > > + while (isdigit(l++[0]) ) { > > + lenl++; > > + } > > + while (isdigit(r++[0]) ) { > > + lenr++; > > + } > > + if (lenl==lenr) { > > + return (firstl - firstr); > > + } else { > > + return (lenl - lenr); > > + } > > I see what's going on here, but I find saving the first mismatching > characters (btw, you could have just saved the difference) then > continuing to advance the l/r pointers a bit confusing at first. Why > not instead do: > > while (isdigit(l[lenl])) lenl++; That would be clearer and more efficient. -- Isaac Dunham --Multipart=_Mon__18_Feb_2013_17_15_58_-0800_C4T4O9M+90oUF7fC Content-Type: text/x-diff; name="strverscmp.diff" Content-Disposition: attachment; filename="strverscmp.diff" Content-Transfer-Encoding: 7bit diff --git a/src/string/strverscmp.c b/src/string/strverscmp.c index 7054967..33a42ee 100644 --- a/src/string/strverscmp.c +++ b/src/string/strverscmp.c @@ -1,7 +1,40 @@ +#define _GNU_SOURCE +#include #include +#include int strverscmp(const char *l, const char *r) { - /* FIXME */ - return strcmp(l, r); + int haszero=1; + while (*l==*r) { + if (!*l) return 0; + + if (*l=='0') { + if (haszero==1) { + haszero=0; + } + } else if (isdigit(*l)) { + if (haszero==1) { + haszero=2; + } + } else { + haszero=1; + } + l++; r++; + } + if (haszero==1 && (*l=='0' || *r=='0')) { + haszero=0; + } + if ((isdigit(*l) && isdigit(*r) ) && haszero) { + size_t lenl=0, lenr=0; + while (isdigit(l[lenl]) ) lenl++; + while (isdigit(r[lenr]) ) lenr++; + if (lenl==lenr) { + return (*l - *r); + } else { + return (lenl - lenr); + } + } else { + return (*l - *r); + } } --Multipart=_Mon__18_Feb_2013_17_15_58_-0800_C4T4O9M+90oUF7fC--