You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by "William A. Rowe, Jr." <wr...@rowe-clan.net> on 2001/04/11 08:30:17 UTC
Buckets code question
Have finished win32 vc5's equivilant of -wall (/W4) ... found many patches to
apply shortly (some my own oversights), but here's one odd one I discovered.
Oh buckets macro designer, is this is what we expected?
apr-util\buckets\apr_buckets_socket.c(142) : warning C4702: unreachable code
apr-util\buckets\apr_buckets_simple.c(115) : warning C4702: unreachable code
apr-util\buckets\apr_buckets_simple.c(149) : warning C4702: unreachable code
apr-util\buckets\apr_buckets_pool.c(159) : warning C4702: unreachable code
apr-util\buckets\apr_buckets_pipe.c(147) : warning C4702: unreachable code
apr-util\buckets\apr_buckets_mmap.c(111) : warning C4702: unreachable code
apr-util\buckets\apr_buckets_heap.c(120) : warning C4702: unreachable code
apr-util\buckets\apr_buckets_flush.c(84) : warning C4702: unreachable code
apr-util\buckets\apr_buckets_file.c(195) : warning C4702: unreachable code
apr-util\buckets\apr_buckets_eos.c(84) : warning C4702: unreachable code
Hopefully :-)
Bill
Re: Buckets code question
Posted by cm...@collab.net.
Justin Erenkrantz <je...@ebuilt.com> writes:
> If I understand this snippet correctly, why do we even have the do while
> loop in the first place?
I present you with a comment near a similar macro (SVN_ERR) used in
Subversion.
/* The `do { ... } while (0)' wrapper has no semantic effect, but it
makes this macro syntactically equivalent to the expression
statement it resembles. Without it, statements like
if (a)
SVN_ERR (some operation);
else
foo;
would not mean what they appear to. */
Re: Buckets code question
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Wed, Apr 11, 2001 at 07:32:10AM -0700, rbb@covalent.net wrote:
> That looks like a bug in the compiler. That code is definately reachable.
I'd bet that the warning is related to the macro expansion.
#define apr_bucket_do_create(do_make) \
do { \
apr_bucket *b, *ap__b; \
b = calloc(1, sizeof(*b)); \
if (b == NULL) { \
return NULL; \
} \
ap__b = do_make; \
if (ap__b == NULL) { \
free(b); \
return NULL; \
} \
APR_RING_ELEM_INIT(ap__b, link); \
return ap__b; \
} while(0)
I bet VC++ is complaining that the while(0) can't be reached.
If I understand this snippet correctly, why do we even have the do while
loop in the first place?
My $.02. -- justin
Re: Buckets code question
Posted by Cliff Woolley <cl...@yahoo.com>.
On Wed, 11 Apr 2001, William A. Rowe, Jr. wrote:
> Oh buckets macro designer, is this is what we expected?
>
> apr-util\buckets\apr_buckets_socket.c(142) : warning C4702: unreachable code
> ...
Not exactly. I've never seen a compiler complain about this macro before.
To save the others a little homework, it's complaining about
apr_bucket_do_create(), which is a macro that looks like this:
do { \
apr_bucket *b, *ap__b; \
b = calloc(1, sizeof(*b)); \
if (b == NULL) { \
return NULL; \
} \
ap__b = do_make; \
if (ap__b == NULL) { \
free(b); \
return NULL; \
} \
APR_RING_ELEM_INIT(ap__b, link); \
return ap__b; \
} while(0)
The only "unreachable code" that I see is the while(0), which is supposed
to get compiled away anyhow. do {} while(0) is the preferred way to do
multiline macros (to allow the macro user to place a semicolon after the
macro invocation without causing a syntax error), so I'll agree with
Ryan: it's a bug in your compiler.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: Buckets code question
Posted by Greg Marr <gr...@alum.wpi.edu>.
At 10:51 AM 04/11/2001, William A. Rowe, Jr. wrote:
>APU_DECLARE(apr_bucket *) apr_bucket_socket_create(apr_socket_t *p)
>{
> do {
>[...]
> return ap__b;
> } while(0);
>}
>
>So the final While(0); is definately unreachable. No compiler error.
>
>My only question, why do {} while(0); rather than {} ?
As Cliff said:
(to allow the macro user to place a semicolon after the macro
invocation without causing a syntax error)
If the user did this:
if(condition)
MACRO(params);
else
MACRO(params);
Using {} would expand to this:
if(condition)
{
}
;
else
{
}
;
while the other form expands to this
if(condition)
do {
} while(0);
else
do {
} while(0);
which is fine.
Of course, it doesn't matter if the condition is always enclosed in
braces, but the compiler won't enforce that, and you can end up with
error messages that don't make sense if you don't include the braces.
--
Greg Marr
gregm@alum.wpi.edu
"We thought you were dead."
"I was, but I'm better now." - Sheridan, "The Summoning"
Re: Buckets code question
Posted by Bill Stoddard <bi...@wstoddard.com>.
Much better. Macros are like C++ multiple inheritance. There are times and
places for them but generally you are better off without them :-)
Bill
> On Wed, 11 Apr 2001 rbb@covalent.net wrote:
>
> > > #define apr_bucket_do_create(ap__b,do_make) \
> > > do { \
> > > apr_bucket *b; \
> > > b = calloc(1, sizeof(*b)); \
> > > ap__b = do_make; \
> > > APR_RING_ELEM_INIT(ap__b, link); \
> > > } while(0)
> >
> > +1
>
> Upon further inspection, the also magical apr_bucket *b is also now
> unnecessary since ap__b is no longer tested for NULL. So
> apr_bucket_do_create() collapses even further down to just:
>
> #define apr_bucket_do_create(ap__b,do_make) \
> do { \
> ap__b = calloc(1, sizeof(*ap__b)); \
> ap__b = do_make; \
> APR_RING_ELEM_INIT(ap__b, link); \
> } while(0)
>
> And callers do something like this:
>
> {
> apr_bucket *b;
>
> apr_bucket_do_create(b, apr_bucket_foo_make(b, foo));
> return b;
> }
>
> But this is screwy for two reasons: the double assignments to ap__b in the
> macro and the seeming call to rv = apr_bucket_foo_make(b, foo) BEFORE
> calling apr_bucket_do_create(b, rv) (if apr_bucket_do_create() were a
> function).
>
> It seems to me that apr_bucket_do_create() has outlived its usefulness.
> It's only a few lines long at this point, which are less tricky-looking
> when done inline than when done as a macro. So what I'm going to do
> unless someone objects strongly is to nix it altogether and just do this:
>
>
> #define APR_BUCKET_INIT(b) APR_RING_ELEM_INIT((b), link)
>
> APU_DECLARE(apr_bucket *) apr_bucket_foo_create(apr_foo_t *foo)
> {
> apr_bucket *b = (apr_bucket *)calloc(1, sizeof(*b));
>
> APR_BUCKET_INIT(b);
> return apr_bucket_foo_make(b, foo);
> }
>
>
> Note that I've switched the effective order of APR_BUCKET_INIT() and
> apr_bucket_foo_make, but it shouldn't matter since the two never touch
> each other's data (and if they did, it would be better for the init to
> happen first anyway).
>
> --Cliff
>
>
> --------------------------------------------------------------
> Cliff Woolley
> cliffwoolley@yahoo.com
> Charlottesville, VA
>
>
Re: Buckets code question
Posted by Cliff Woolley <cl...@yahoo.com>.
On Wed, 11 Apr 2001 rbb@covalent.net wrote:
> > #define apr_bucket_do_create(ap__b,do_make) \
> > do { \
> > apr_bucket *b; \
> > b = calloc(1, sizeof(*b)); \
> > ap__b = do_make; \
> > APR_RING_ELEM_INIT(ap__b, link); \
> > } while(0)
>
> +1
Upon further inspection, the also magical apr_bucket *b is also now
unnecessary since ap__b is no longer tested for NULL. So
apr_bucket_do_create() collapses even further down to just:
#define apr_bucket_do_create(ap__b,do_make) \
do { \
ap__b = calloc(1, sizeof(*ap__b)); \
ap__b = do_make; \
APR_RING_ELEM_INIT(ap__b, link); \
} while(0)
And callers do something like this:
{
apr_bucket *b;
apr_bucket_do_create(b, apr_bucket_foo_make(b, foo));
return b;
}
But this is screwy for two reasons: the double assignments to ap__b in the
macro and the seeming call to rv = apr_bucket_foo_make(b, foo) BEFORE
calling apr_bucket_do_create(b, rv) (if apr_bucket_do_create() were a
function).
It seems to me that apr_bucket_do_create() has outlived its usefulness.
It's only a few lines long at this point, which are less tricky-looking
when done inline than when done as a macro. So what I'm going to do
unless someone objects strongly is to nix it altogether and just do this:
#define APR_BUCKET_INIT(b) APR_RING_ELEM_INIT((b), link)
APU_DECLARE(apr_bucket *) apr_bucket_foo_create(apr_foo_t *foo)
{
apr_bucket *b = (apr_bucket *)calloc(1, sizeof(*b));
APR_BUCKET_INIT(b);
return apr_bucket_foo_make(b, foo);
}
Note that I've switched the effective order of APR_BUCKET_INIT() and
apr_bucket_foo_make, but it shouldn't matter since the two never touch
each other's data (and if they did, it would be better for the init to
happen first anyway).
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: Buckets code question
Posted by rb...@covalent.net.
>
> I'll consider that three +1's on concept... me, Greg Marr (yes?), and
> FirstBill. I'll touch this up and commit it this afternoon. I'd also
> like to remove the return NULL; lines in the macro. There are two ways to
> do that:
>
> 1) ap__b = NULL; continue;
> 2) get rid of the malloc failure checking entirely
> 3) ignore this issue and leave the return NULL's in there.
>
> I prefer #1, but I predict being overruled by the group on this since many
> of you think it's better to segfault than attempt to gracefully handle
> malloc failures. So assuming #2 is preferred by the group, does everyone
> like the following as the new apr_bucket_do_create macro?
>
> #define apr_bucket_do_create(ap__b,do_make) \
> do { \
> apr_bucket *b; \
> b = calloc(1, sizeof(*b)); \
> ap__b = do_make; \
> APR_RING_ELEM_INIT(ap__b, link); \
> } while(0)
+1
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: Buckets code question
Posted by Cliff Woolley <cl...@yahoo.com>.
On Wed, 11 Apr 2001, Greg Marr wrote:
> I agree. Having macros that look like functions, but have return
> statements in them, is bad. It also prevents those macros from being
> made into inline functions without changing all the places that call
> them.
I'll consider that three +1's on concept... me, Greg Marr (yes?), and
FirstBill. I'll touch this up and commit it this afternoon. I'd also
like to remove the return NULL; lines in the macro. There are two ways to
do that:
1) ap__b = NULL; continue;
2) get rid of the malloc failure checking entirely
3) ignore this issue and leave the return NULL's in there.
I prefer #1, but I predict being overruled by the group on this since many
of you think it's better to segfault than attempt to gracefully handle
malloc failures. So assuming #2 is preferred by the group, does everyone
like the following as the new apr_bucket_do_create macro?
#define apr_bucket_do_create(ap__b,do_make) \
do { \
apr_bucket *b; \
b = calloc(1, sizeof(*b)); \
ap__b = do_make; \
APR_RING_ELEM_INIT(ap__b, link); \
} while(0)
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: Buckets code question
Posted by Greg Marr <gr...@alum.wpi.edu>.
At 11:50 AM 04/11/2001, Cliff Woolley wrote:
>While this causes some very minor redundancy in the
>apr_bucket_foo_create() functions, it has several benefits:
>
>(1) both warnings and errors eliminated
>(2) the magical return is made explicit, which makes
> apr_bucket_foo_create() easier to understand for someone not
> familiar with apr_bucket_do_create()'s innards.
I agree. Having macros that look like functions, but have return
statements in them, is bad. It also prevents those macros from being
made into inline functions without changing all the places that call
them.
--
Greg Marr
gregm@alum.wpi.edu
"We thought you were dead."
"I was, but I'm better now." - Sheridan, "The Summoning"
Re: Buckets code question
Posted by Bill Stoddard <bi...@wstoddard.com>.
>
> Our problem, obviously, is the explicit return within the block. Maybe we
> could change the call to look like this:
>
> {
> apr_bucket *the_new_bucket;
>
> apr_bucket_do_create(the_new_bucket, ...);
> return the_new_bucket;
> }
>
+1. I really like the explicit return. Returning out of a function call from
within a macro should be a shooting offense.
Bill
Re: Buckets code question
Posted by Cliff Woolley <cl...@yahoo.com>.
On Wed, 11 Apr 2001, William A. Rowe, Jr. wrote:
> } while (0);
> return ap__b;
> } while(0);
> }
>
> So the final While(0); is definately unreachable. No compiler error.
>
> My only question, why do {} while(0); rather than {} ?
It's not do {} while(0); rather than {} , it's that rather than {}; .
The semicolon is added by the caller of apr_bucket_do_create(), who treats
it as a function call.
apr_bucket_do_create(); <---the semicolon
{}; is a syntax error on some platforms because empty statements are not
allowed, and here the semicolon is essentially delimiting an empty
statement which occurs between the closing brace and the semicolon.
do {} while(0); is actually the preferred way to get around this issue,
since it gets compiled away by an optimizing compiler anyhow.
Our problem, obviously, is the explicit return within the block. Maybe we
could change the call to look like this:
{
apr_bucket *the_new_bucket;
apr_bucket_do_create(the_new_bucket, ...);
return the_new_bucket;
}
And adjust apr_bucket_do_create to look like this:
#define apr_bucket_do_create(ap__b,do_make) \
do { \
apr_bucket *b; \
b = calloc(1, sizeof(*b)); \
if (b == NULL) { \
return NULL; \
} \
ap__b = do_make; \
if (ap__b == NULL) { \
free(b); \
return NULL; \
} \
APR_RING_ELEM_INIT(ap__b, link); \
} while(0)
While this causes some very minor redundancy in the
apr_bucket_foo_create() functions, it has several benefits:
(1) both warnings and errors eliminated
(2) the magical return is made explicit, which makes
apr_bucket_foo_create() easier to understand for someone not
familiar with apr_bucket_do_create()'s innards.
Thoughts?
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: Buckets code question
Posted by "William A. Rowe, Jr." <ad...@rowe-clan.net>.
----- Original Message -----
From: <rb...@covalent.net>
To: "William A. Rowe, Jr." <wr...@rowe-clan.net>
Cc: <de...@apr.apache.org>
Sent: Wednesday, April 11, 2001 9:32 AM
Subject: Re: Buckets code question
> On Wed, 11 Apr 2001, William A. Rowe, Jr. wrote:
>
> > Have finished win32 vc5's equivilant of -wall (/W4) ... found many patches to
> > apply shortly (some my own oversights), but here's one odd one I discovered.
> >
> > Oh buckets macro designer, is this is what we expected?
> >
> > apr-util\buckets\apr_buckets_socket.c(142) : warning C4702: unreachable code
> > apr-util\buckets\apr_buckets_simple.c(115) : warning C4702: unreachable code
> > apr-util\buckets\apr_buckets_simple.c(149) : warning C4702: unreachable code
> > apr-util\buckets\apr_buckets_pool.c(159) : warning C4702: unreachable code
> > apr-util\buckets\apr_buckets_pipe.c(147) : warning C4702: unreachable code
> > apr-util\buckets\apr_buckets_mmap.c(111) : warning C4702: unreachable code
> > apr-util\buckets\apr_buckets_heap.c(120) : warning C4702: unreachable code
> > apr-util\buckets\apr_buckets_flush.c(84) : warning C4702: unreachable code
> > apr-util\buckets\apr_buckets_file.c(195) : warning C4702: unreachable code
> > apr-util\buckets\apr_buckets_eos.c(84) : warning C4702: unreachable code
> >
> > Hopefully :-)
>
> That looks like a bug in the compiler. That code is definately reachable.
APU_DECLARE(apr_bucket *) apr_bucket_socket_create(apr_socket_t *p)
{
apr_bucket_do_create(apr_bucket_socket_make(b, p));
}
---expands to---
APU_DECLARE(apr_bucket *) apr_bucket_socket_create(apr_socket_t *p)
{
do {
apr_bucket *b, *ap__b;
b = calloc(1, sizeof(*b));
if (b == NULL) {
return NULL;
}
ap__b = apr_bucket_socket_make(b, p);
if (ap__b == NULL) {
free(b);
return NULL;
}
APR_RING_ELEM_INIT(ap__b, link);
return ap__b;
} while(0);
}
---expands to---
APU_DECLARE(apr_bucket *) apr_bucket_socket_create(apr_socket_t *p)
{
do {
apr_bucket *b, *ap__b;
b = calloc(1, sizeof(*b));
if (b == NULL) {
return NULL;
}
ap__b = apr_bucket_socket_make(b, p);
if (ap__b == NULL) {
free(b);
return NULL;
}
do {
((ap__b))->link.next = (ap__b);
((ap__b))->link.prev = (ap__b);
} while (0);
return ap__b;
} while(0);
}
So the final While(0); is definately unreachable. No compiler error.
My only question, why do {} while(0); rather than {} ?
Bill
Re: Buckets code question
Posted by Dirk-Willem van Gulik <di...@covalent.net>.
On Wed, 11 Apr 2001 rbb@covalent.net wrote:
> On Wed, 11 Apr 2001, William A. Rowe, Jr. wrote:
>
> > apr-util\buckets\apr_buckets_eos.c(84) : warning C4702: unreachable code
> >
> > Hopefully :-)
>
> That looks like a bug in the compiler. That code is definately reachable.
I struggled with that as well as my compiler says the same thing; if you
change the
...
return foo;
} while(0);
into an if (a) return foo; it goes away. While on the subject - I am
wondering if the return foo would not also break inline/optimize rq's.
Dw
Re: Buckets code question
Posted by rb...@covalent.net.
On Wed, 11 Apr 2001, William A. Rowe, Jr. wrote:
> Have finished win32 vc5's equivilant of -wall (/W4) ... found many patches to
> apply shortly (some my own oversights), but here's one odd one I discovered.
>
> Oh buckets macro designer, is this is what we expected?
>
> apr-util\buckets\apr_buckets_socket.c(142) : warning C4702: unreachable code
> apr-util\buckets\apr_buckets_simple.c(115) : warning C4702: unreachable code
> apr-util\buckets\apr_buckets_simple.c(149) : warning C4702: unreachable code
> apr-util\buckets\apr_buckets_pool.c(159) : warning C4702: unreachable code
> apr-util\buckets\apr_buckets_pipe.c(147) : warning C4702: unreachable code
> apr-util\buckets\apr_buckets_mmap.c(111) : warning C4702: unreachable code
> apr-util\buckets\apr_buckets_heap.c(120) : warning C4702: unreachable code
> apr-util\buckets\apr_buckets_flush.c(84) : warning C4702: unreachable code
> apr-util\buckets\apr_buckets_file.c(195) : warning C4702: unreachable code
> apr-util\buckets\apr_buckets_eos.c(84) : warning C4702: unreachable code
>
> Hopefully :-)
That looks like a bug in the compiler. That code is definately reachable.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------