apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ryan Bloom <rbl...@gmail.com>
Subject Re: [PATCH] Fix testing of "BEOS" symbol
Date Tue, 16 Nov 2004 02:45:21 GMT
I'm not saying who is correct here, but in the _VERY_ early days of
APR (back before we had our own mailing list), we had a discussion
about this very topic.  The goal back then was to determine if #if
could be used instead of #ifdef or #if defined().  We decided back
then to always use #if instead of #ifdef.

Obviously, this hasn't been maintained, but IIRC, that was the
original code style that we decided on.

Ryan


On Tue, 16 Nov 2004 01:41:43 +0000, Julian Foad
<julianfoad@btopenworld.com> wrote:
> 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
> 


-- 
Ryan Bloom
rbb@apache.org
rbb@redhat.com
rbloom@gmail.com

Mime
View raw message