From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44144) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RhMmZ-0002B6-QD for qemu-devel@nongnu.org; Sun, 01 Jan 2012 09:53:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RhMmY-00032A-Se for qemu-devel@nongnu.org; Sun, 01 Jan 2012 09:53:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49944) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RhMmY-000326-M3 for qemu-devel@nongnu.org; Sun, 01 Jan 2012 09:53:06 -0500 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 q01Er5vS023048 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 1 Jan 2012 09:53:05 -0500 Message-ID: <4F00734D.8020008@redhat.com> Date: Sun, 01 Jan 2012 16:53:01 +0200 From: Avi Kivity MIME-Version: 1.0 References: <1321898431-18449-1-git-send-email-pbonzini@redhat.com> <1321898431-18449-2-git-send-email-pbonzini@redhat.com> In-Reply-To: <1321898431-18449-2-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/4] rtc: fix 12-hour mode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org On 11/21/2011 08:00 PM, Paolo Bonzini wrote: > Hours in 12-hour mode are in the 1-12 range, not 0-11. > > @@ -320,7 +324,8 @@ static void rtc_copy_date(RTCState *s) > s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour); > } else { > /* 12 hour format */ > - s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, tm->tm_hour % 12); > + int h = (tm->tm_hour % 12) ? tm->tm_hour % 12 : 12; > + s->cmos_data[RTC_HOURS] = rtc_to_bcd(s, h); > if (tm->tm_hour >= 12) > s->cmos_data[RTC_HOURS] |= 0x80; > } Nitpick, don't update patch on this account: I dislike seeing int-to-bool conversion on anything that is not a true/false or count value. Things like if (has_some_property) or if (!nr_items) read well and are easily understood. But here, you're not checking for "are there any tm_hours, if yes, use them, if not, use 12". You're testing against the value 0 which has a special encoding in 12 hour mode. The is usually manifested in if (!strcmp(a, b)) ... strcmp() does not return a bool or a count, and in fact it reads exactly the opposite of the intent: "if not string compare". strcmp() returns an enumeration, or perhaps a mapping of string trichotomy to integer trichotomy. Sorry about the pontification, back to the regularly scheduled whitespace discussion. -- error compiling committee.c: too many arguments to function