Message ID | c087928d-c0a6-4121-8236-84a1a9e59870@BAMAIL02.ba.imgtec.org |
---|---|
State | Committed |
Headers |
Return-Path: <x14307373@homiemail-mx20.g.dreamhost.com> X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx20.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id CD992360075 for <siddhesh@wilcox.dreamhost.com>; Mon, 28 Apr 2014 10:38:46 -0700 (PDT) Received: by homiemail-mx20.g.dreamhost.com (Postfix, from userid 14307373) id 801E941627BB5; Mon, 28 Apr 2014 10:38:46 -0700 (PDT) X-Original-To: glibc@patchwork.siddhesh.in Delivered-To: x14307373@homiemail-mx20.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx20.g.dreamhost.com (Postfix) with ESMTPS id 5D36741627B8F for <glibc@patchwork.siddhesh.in>; Mon, 28 Apr 2014 10:38:46 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:date:to:subject:mime-version:content-type :content-transfer-encoding:message-id; q=dns; s=default; b=EeNjl 6qrw1yFbFTB6HAa1h1c3QQ84x8ZTLrI21nyGrq/6OAgKk1UomXal/zPHbFN/l/I+ xsK/c1EV40DiOp5BvpbQ0s68sW5TJXdCmKrTacvaEQfmI7aWbs+gu4CbPBLECWIY +FHYYI8zau97OAJbHunmMIws/KOWQbXTZ3PvSw= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:date:to:subject:mime-version:content-type :content-transfer-encoding:message-id; s=default; bh=NIs/SgDlEg6 WOAcA54gPCFnPsLQ=; b=CfX0PcDq9Wg7npdoQZiVRm1+Ig5M02sYzFQF7Gd3TOc IpfSQf9JRhgnjiZPYlFV1lLQQJjFw9VkQ+OI6phQBskHitBT+pfVYn86+3ghbOAp v+zAXkp0Jhz3beEb2ZUKzELwF5c4ZMEVhMl4DGy7y56xnYPpL8NkavM03XqpMIrE = Received: (qmail 17763 invoked by alias); 28 Apr 2014 17:38:44 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-glibc=patchwork.siddhesh.in@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 17751 invoked by uid 89); 28 Apr 2014 17:38:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mailapp01.imgtec.com From: "Steve Ellcey " <sellcey@mips.com> Date: Mon, 28 Apr 2014 10:38:35 -0700 To: <libc-alpha@sourceware.org> Subject: [Patch] Fix ONE_DIRECTION undef warnings. User-Agent: Heirloom mailx 12.5 6/20/10 MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-ID: <c087928d-c0a6-4121-8236-84a1a9e59870@BAMAIL02.ba.imgtec.org> X-DH-Original-To: glibc@patchwork.siddhesh.in |
Commit Message
Steve Ellcey
April 28, 2014, 5:38 p.m. UTC
Here is another attempt to fix some undef warnings. The only use of ONE_DIRECTION is in iconv/skeleton.c and the only setting of it is in iconv/gconv_simple.c (where it is set to 1). This patch checks (in skeleton.c) to see if it is set and then sets it to 0 if it is not. The use of it, later in skeleton.c is already done with a '#if' so nothing needed to be changed there. Tested on mips-mti-linux-gnu, the only code change was to the line number info in the assert calls. Steve Ellcey sellcey@mips.com 2014-04-28 Steve Ellcey <sellcey@mips.com> * iconf/skelenton.c (ONE_DIRECTION): Set default value if not set.
Comments
OK
On 04/28/2014 01:41 PM, Roland McGrath wrote:
> OK
Would it have been better to clarify the interface and have all
the converters define ONE_DIRECTION as either 0 or 1, and that way
we have *explicit* definition of intent from all the converters
including iconv/skeleton.c? With no `ifndef' in skeleton.c which
has most of the same problems as we had before?
Cheers,
Carlos.
On Tue, 2014-04-29 at 15:33 -0400, Carlos O'Donell wrote: > On 04/28/2014 01:41 PM, Roland McGrath wrote: > > OK > > Would it have been better to clarify the interface and have all > the converters define ONE_DIRECTION as either 0 or 1, and that way > we have *explicit* definition of intent from all the converters > including iconv/skeleton.c? With no `ifndef' in skeleton.c which > has most of the same problems as we had before? > > Cheers, > Carlos. I think my main issue with this is how many new (duplicate) definitions it adds. This is even more of an issue with something like HP_SMALL_TIMING_AVAIL. Only one platform (alpha) ever sets this to 1. But with the current setup, to ensure it always has a value, we have to define it to 0 in 9 different hp-timing.h headers. That replication bothers me and I would like to have one default value defined somewhere that could be included by the platform specific hp-timing.h files instead of defining it in all 9 of the non-alpha hp-timing.h header files. But there doesn't seem to be an existing infrastructure for that, and I am not sure if a patch to create such a setup would be accepted and I don't know if it should be designed just for hp-timing.h or if it should be a more global header file that other platform headers could also include to provide default values for other macros. Steve Ellcey sellcey@mips.com
On Tue, 29 Apr 2014, Steve Ellcey wrote: > I think my main issue with this is how many new (duplicate) definitions > it adds. This is even more of an issue with something like > HP_SMALL_TIMING_AVAIL. Only one platform (alpha) ever sets this to 1. > But with the current setup, to ensure it always has a value, we have to > define it to 0 in 9 different hp-timing.h headers. That replication > bothers me and I would like to have one default value defined somewhere > that could be included by the platform specific hp-timing.h files > instead of defining it in all 9 of the non-alpha hp-timing.h header > files. But there doesn't seem to be an existing infrastructure for > that, and I am not sure if a patch to create such a setup would be > accepted and I don't know if it should be designed just for hp-timing.h > or if it should be a more global header file that other platform headers > could also include to provide default values for other macros. I don't see a problem with using #include_next in the hp-timing.h case, to include generic defaults then override them selectively.
On 04/29/2014 04:11 PM, Steve Ellcey wrote: > On Tue, 2014-04-29 at 15:33 -0400, Carlos O'Donell wrote: >> On 04/28/2014 01:41 PM, Roland McGrath wrote: >>> OK >> >> Would it have been better to clarify the interface and have all >> the converters define ONE_DIRECTION as either 0 or 1, and that way >> we have *explicit* definition of intent from all the converters >> including iconv/skeleton.c? With no `ifndef' in skeleton.c which >> has most of the same problems as we had before? >> >> Cheers, >> Carlos. > > I think my main issue with this is how many new (duplicate) definitions > it adds. This is even more of an issue with something like > HP_SMALL_TIMING_AVAIL. Only one platform (alpha) ever sets this to 1. > But with the current setup, to ensure it always has a value, we have to > define it to 0 in 9 different hp-timing.h headers. That replication > bothers me and I would like to have one default value defined somewhere > that could be included by the platform specific hp-timing.h files > instead of defining it in all 9 of the non-alpha hp-timing.h header > files. But there doesn't seem to be an existing infrastructure for > that, and I am not sure if a patch to create such a setup would be > accepted and I don't know if it should be designed just for hp-timing.h > or if it should be a more global header file that other platform headers > could also include to provide default values for other macros. That is a *very* good question and you raise a valid point. I'm not against a master "default" hp-timing.h that does: /* This is the default... */ #define HP_SMALL_TIMING_AVAIL 0 I'm also not bothered by having each hp-timing.h define it either. The HP_SMALL_TIMING_AVAIL is a statement of interface that each of the targets should be making. By using ifdef we run the risk of targets getting unintended semantics. It's exactly the problem we're trying to avoid. We want the machine maintainers to make that conscious choice when they setup the low-level support e.g. "Do I or Don't I support X, Y, and Z?" That conscious decision can be either: #define HP_SMALL_TIMING_AVAIL 0 or #include <hp-timing-defaults.h> Part of the problem with fixing these issues is setting up a good "best practice" for this kind of fix. I am all for having a common hp-timing.h that you can include with #include_next <...>. Then Alpha does: /* Our foo is different. */ #undef FOO #define FOO I was looking at a similar pattern for unifying Linux UAPI constants. I think that a single mega-header for all these kinds of constants would be overkill. We might refactor up to one header per sub-system. Lastly, I hope that we can all agree that: #ifndef ONE_DIRECTION #define ONE_DIRECTION #endif Isn't quite what we want since it silently accepts a particular configuration. We also didn't do a good job of documenting this particular default :-( What we want is: #define ONE_DIRECTION 0 Either compiles and you get the semantics you want, or it fails to compile with an error. Similarly if all uses are #if, then you get the -Wundef error for using something you didn't define. Does that answer your question? Cheers, Carlos.
diff --git a/iconv/skeleton.c b/iconv/skeleton.c index c3f161a..1908949 100644 --- a/iconv/skeleton.c +++ b/iconv/skeleton.c @@ -163,6 +163,10 @@ # endif #endif +#ifndef ONE_DIRECTION +# define ONE_DIRECTION 0 +#endif + /* How many bytes are needed at most for the from-charset. */ #ifndef MAX_NEEDED_FROM