apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kfo...@collab.net
Subject Re: [PATCH] Fix testing of "BEOS" symbol
Date Tue, 16 Nov 2004 03:26:53 GMT
Ryan Bloom <rbloom@gmail.com> writes:
> 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.

I think whatever the prevailing trend in the current code is, should
be Julian's guide.  So far, he's the only one who's done the research
to determine that trend, AFAIK.  (Since it doesn't objectively matter
very much, might as well make the minority conform to the majority.)

Of course, this is truly a bikeshed.  Therefore, this is my last post
on the matter :-).

-K

> 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