httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules:
Date Tue, 04 Oct 2005 19:32:20 GMT
Brian Candler wrote:
> 
> Hmm. Given the example
> 
>     pid = (pid_t)((long)apr_hash_get(script_hash, &cgid_req.conn_id, sizeof(cgid_req.conn_id)));
> 
> then it's no good declaring variable 'pid' to be of type apr_intptr_t rather
> than pid_t, because somewhere down the line it will be passed to a function
> which expects a pid_t argument, and you'll get a warning there instead.

     pid = (pid_t)apr_hash_get(script_hash, &cgid_req.conn_id,
                               sizeof(cgid_req.conn_id));

would be correct in -this- case.  We have to make a small concession
to the platform architects, that there's no sense in more pid's than
you could count with a void*, so no matter if pid_t is an int or long
(or a handle) this simply works.  Now if the line above produces our
compile warning, of truncation, there's no issue I suppose with

      pid = (pid_t)((ap_ptrint_t)apr_hash_get(script_hash,
                                     &cgid_req.conn_id,
                                     sizeof(cgid_req.conn_id)));

or perhaps we simplify, with AP_INTPTR(n) and AP_PTRINT(p) macros?

      pid = (pid_t)AP_PTRINT(apr_hash_get(script_hash,
                                     &cgid_req.conn_id,
                                     sizeof(cgid_req.conn_id));

e.g.
#define AP_INTPTR(n) ((void*)((ap_ptrint_t)(n)))
#define AP_PTRINT(n) ((ap_ptrint_t)(n))

I suppose I understand the truncation warning but...

> Now, Apache has lots of cases where (void *)'s are cast to integral types
> (possibly smaller than a void *) and back again. Since this is intentional,
> I think the real solution is to turn off the compiler warning for this case,
> not to bastardise the code with lots of double-casts. The more unnecessary
> casts you put into your code, the more likely you are to make type errors
> which the compiler can't check.

> IMO, if gcc can't disable this warning, then we need to file a bug report.
> Or else just pipe gcc stderr through
> 
>     egrep -v "warning: cast (to|from) pointer (to|from) integer of different size"
> 
> and forget about it.

Now that's not a healthy situation, -1 on the gcc pipe :)

> P.S. The reverse case is sillier, given that the value is moving to a
> *larger* type and therefore no data loss can occur:
> 
>     short a;
>     long b = a;          // (7) no warning
> 
>     short a;
>     void *b = a;         // (8) warns "pointer from integer without a cast" (ok)
>     void *c = (void *)a; // (9) warns "cast to pointer from integer of different size"
>     void *d = (void *)(long)a;  // (10) even more stupid!!!
> 
> Note that having a 'large enough' integral type in case (10) is not good
> enough. We need to cast 'a' to an integral type which is *exactly* the same
> size as a void *, to silence this particular warning.

I agree with your assertion that the code example above IS a gcc bug.
This is expected in C++, but the waning (9) above is completely bogus,
as you illustrate with line (7) above.

What's your though on AP_PTRINT(n) / AP_INTPTR(p)?

Bill

Mime
View raw message