You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Julian Foad <ju...@btopenworld.com> on 2004/11/16 00:04:15 UTC
[PATCH] Fix testing of "BEOS" symbol
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".
- Julian
Re: [PATCH] Fix testing of "BEOS" symbol
Posted by Julian Foad <ju...@btopenworld.com>.
Joe Orton wrote:
> The fact that -Wundef exposed bugs like apr_signal_block() being a noop
> seems like sufficient justification to make the source -Wundef clean to
> avoid future screwups, I agree with this. But any chance you could fix
> them all rather than picking them off one at at time?
>
> file_io/unix/pipe.c:27:5: warning: "BEOS" is not defined
> file_io/unix/pipe.c:37:6: warning: "BEOS_BLOCKING" is not defined
> file_io/unix/pipe.c:73:6: warning: "BEOS_BLOCKING" is not defined
> file_io/unix/readwrite.c:23:6: warning: "BEOS_R5" is not defined
> support/unix/waitio.c:24:6: warning: "BEOS_R5" is not defined
The reason I didn't fix the rest, which are all BEOS-related, is that I don't
have experience with (or test facilities for) compiling on BEOS, and it is not
so clear how they are intended to be used (def/undef or 0/1). I would
certainly like them to be fixed, and started trying to do so, but I quickly
realised that I was not easily going to achieve a solution of which I was
confident.
The reason I made the one BEOS-related fix that I did is because it appears in
a header file and thus the warning pops up many times, once for each C file
that uses it, and I was able to discover the correct solution from other usage
of the same symbol. Where the same symbol gets a warning in pipe.c:27, it is
in combination with other BEOS-related symbols and I was not able easily to
find a good fix.
So, no, sorry, I'm not going to fix the other BEOS symbols.
- Julian
Re: [PATCH] Fix testing of "BEOS" symbol
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Nov 16, 2004 at 01:30:00PM +0000, Julian Foad wrote:
> Branko Čibej wrote:
> >If the #if was an exception WRT the BEOS symbol, then
> >your change is fine; but you should say so (perhaps you did in another
> >post and I missed it; sorry).
>
> I did say so when I first posted the patch a few days ago, but I should say
> so in the log message. The patch is re-attached with an improved log
> message.
The fact that -Wundef exposed bugs like apr_signal_block() being a noop
seems like sufficient justification to make the source -Wundef clean to
avoid future screwups, I agree with this. But any chance you could fix
them all rather than picking them off one at at time?
file_io/unix/pipe.c:27:5: warning: "BEOS" is not defined
file_io/unix/pipe.c:37:6: warning: "BEOS_BLOCKING" is not defined
file_io/unix/pipe.c:73:6: warning: "BEOS_BLOCKING" is not defined
file_io/unix/readwrite.c:23:6: warning: "BEOS_R5" is not defined
support/unix/waitio.c:24:6: warning: "BEOS_R5" is not defined
Regards,
joe
Re: [PATCH] Fix testing of "BEOS" symbol
Posted by Julian Foad <ju...@btopenworld.com>.
Branko Čibej wrote:
> If the #if was an exception WRT the BEOS symbol, then
> your change is fine; but you should say so (perhaps you did in another
> post and I missed it; sorry).
I did say so when I first posted the patch a few days ago, but I should say so
in the log message. The patch is re-attached with an improved log message.
- Julian
Re: [PATCH] Fix testing of "BEOS" symbol
Posted by Branko Čibej <br...@xbc.nu>.
Julian Foad wrote:
> Brane, you have caught me in zero-tolerance week. Please bear with me
> while I rant again. :-)
:-)
I don't disagree with what you said, I was merely pointing out a (the?)
potential catch. If the #if was an exception WRT the BEOS symbol, then
your change is fine; but you should say so (perhaps you did in another
post and I missed it; sorry).
Regarding #if vs. #ifdef in general, well, you noticed that most macros
generated by configure are tested with #if, even though, IIRC, autoconf
leaves them undefined rather than #defined to 0 if the predicate is
false. If so, then always using -Wundef might actually be harmful
because it would generate lots of false positives that people would
start to ignore, and the rare true positives would slip through.
-- Brane
Re: [PATCH] Fix testing of "BEOS" symbol
Posted by kf...@collab.net.
Ryan Bloom <rb...@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
> <ju...@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
Re: [PATCH] Fix testing of "BEOS" symbol
Posted by Ryan Bloom <rb...@gmail.com>.
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
<ju...@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
Re: [PATCH] Fix testing of "BEOS" symbol
Posted by Julian Foad <ju...@btopenworld.com>.
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
Re: [PATCH] Fix testing of "BEOS" symbol
Posted by Branko Čibej <br...@xbc.nu>.
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.
Imagine what happens if some system header does
#define BEOS 0
The code will suddenly be incorrect.
-- Brane