From dev-return-13101-apmail-apr-dev-archive=apr.apache.org@apr.apache.org Tue Nov 16 01:42:09 2004 Return-Path: Delivered-To: apmail-apr-dev-archive@www.apache.org Received: (qmail 8159 invoked from network); 16 Nov 2004 01:42:09 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (209.237.227.199) by minotaur-2.apache.org with SMTP; 16 Nov 2004 01:42:09 -0000 Received: (qmail 39420 invoked by uid 500); 16 Nov 2004 01:42:08 -0000 Delivered-To: apmail-apr-dev-archive@apr.apache.org Received: (qmail 39374 invoked by uid 500); 16 Nov 2004 01:42:07 -0000 Mailing-List: contact dev-help@apr.apache.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Delivered-To: mailing list dev@apr.apache.org Received: (qmail 39360 invoked by uid 99); 16 Nov 2004 01:42:07 -0000 X-ASF-Spam-Status: No, hits=1.6 required=10.0 tests=DNS_FROM_RFC_ABUSE,DNS_FROM_RFC_POST,SPF_HELO_PASS X-Spam-Check-By: apache.org Received-SPF: neutral (hermes.apache.org: local policy) Message-ID: <41995AD7.7050403@btopenworld.com> Date: Tue, 16 Nov 2004 01:41:43 +0000 From: Julian Foad Reply-To: dev@apr.apache.org User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040113 X-Accept-Language: en-gb, en, en-us MIME-Version: 1.0 To: =?UTF-8?B?QnJhbmtvIMSMaWJlag==?= CC: dev@apr.apache.org Subject: Re: [PATCH] Fix testing of "BEOS" symbol References: <419935EF.5070305@btopenworld.com> <41994526.60303@xbc.nu> In-Reply-To: <41994526.60303@xbc.nu> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Checked: Checked X-Spam-Rating: minotaur-2.apache.org 1.6.2 0/1000/N Brane, you have caught me in zero-tolerance week. Please bear with me while I rant again. :-) Branko Čibej wrote: > Julian Foad wrote: > >> This patch only fixes one instance, but one which is in a header file >> and so is encountered frequently. There are other BEOS-related >> symbols being tested badly in C files which I am not fixing here. I >> found these with "gcc -Wundef". > > I'm not sure this is a good change. As has been said before, it is > perfectly valid in C to test an undefined symbol with #if and expect the > test to behave as if the symbol's value was 0. That said, I don't think > -Wundef should be a standard option, even in maintainer mode. The fact that it is perfectly valid C syntax doesn't mean it was the intended meaning, and judging by other occurrences of it within APR, I don't think that was the intended meaning. Programmers adopt practices (such as usually avoiding testing undefined symbols with "#if SYMBOL") that reflect their higher-level intentions better than if they just obeyed the letter of the C syntax "law" and disregarded all other style considerations. Because certain such practices are well known and respected, tools like compilers can warn (optionally - if the programmer reckons that such practices are generally being followed in his project) about usage which seems to contradict those practices. I don't see why you wouldn't enable these warnings if you are working on a project that generally follows this practice. Now, APR does not currently fully follow that practice, but it mostly does, and the benefit comes in as follows: by enabling that warning, I very quickly discovered a place where APR was testing a symbol that it thought was defined (APR_HAS_SIGACTION - see my fourth patch in this mini-series of six) which was not in fact defined. By investigating this, I think I found that that is a bug, and it meant to test "APR_HAVE_SIGACTION" instead. Now, it is possible that I am wrong about that particular "SIGACTION" test (I hope somebody will review it) but in general the ability to detect such bugs is a useful benefit of always testing definedness with "#ifdef" or "#if defined()". > Imagine what happens if some system header does > > #define BEOS 0 > > The code will suddenly be incorrect. Nearly all other tests for that symbol in APR test it with "#ifdef BEOS" or "#if defined(BEOS)". I don't know the intended meaning of that symbol, but I strongly suspect it is intended to be either defined or undefined. Please correct me if I am wrong. Note that I am not saying we should disobey the C syntax - of course we can't - but rather that it is not a good idea to exploit every crevice of it. There are significant benefits to being conservative and consistent in usage of C syntax, and employing tools (e.g. warnings) which are not checking for conformance with the C standard but are checking for particular styles of C usage that we choose to uphold in order to better avoid bugs. I happen to advocate such practices to a greater extent than it seems you would choose to. We have to work to a common consensus of style within a given project. If you, and the project as a whole, choose generally to test undefined symbols with plain "#if", then I can only turn off or ignore the warnings and accept that. But it seems that the practice of explicitly testing for definedness is generally followed in Subversion and in APR so I enable the warnings and investigate any anomalies. I hope this makes sense. - Julian