apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Julian Foad <julianf...@btopenworld.com>
Subject Re: [PATCH] Fix testing of "BEOS" symbol
Date Tue, 16 Nov 2004 01:41:43 GMT
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

Mime
View raw message