You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by Jim Jagielski <ji...@jaguNET.com> on 2005/10/03 13:50:39 UTC
Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules: dav/fs/dbm.c
Just some lines that caught my eye:
>
> - *context = (void *)(value == 'T');
> + *context = (void *)((long)(value == 'T'));
>
> - int value = context != NULL;
> + long value = context != NULL;
> apr_fileperms_t perms = resource->info->finfo.protection;
> - int old_value = (perms & APR_UEXECUTE) != 0;
> + long old_value = (perms & APR_UEXECUTE) != 0;
>
Huh? Whenever I see conditionals cast to (long) I get
suspicious.
>
> - pid = (pid_t)apr_hash_get(script_hash, &cgid_req.conn_id, sizeof(cgid_req.conn_id));
> + pid = (pid_t)((long)apr_hash_get(script_hash, &cgid_req.conn_id, sizeof(cgid_req.conn_id)));
> apr_hash_set(script_hash, key, sizeof(cgid_req.conn_id),
> - (void *)procnew->pid);
> + (void *)((long)procnew->pid));
> }
> {
> - int fd = (int)thefd;
> + int fd = (int)((long)thefd);
>
> else {
> - apr_pool_cleanup_register(r->pool, (void *)sd, close_unix_socket,
> - apr_pool_cleanup_null);
> + apr_pool_cleanup_register(r->pool, (void *)((long)sd),
> + close_unix_socket, apr_pool_cleanup_null);
> break; /* we got connected! */
> }
Same when I see sequential casts... What's the rationale for them?
--
=======================================================================
Jim Jagielski [|] jim@jaguNET.com [|] http://www.jaguNET.com/
"If you can dodge a wrench, you can dodge a ball."
Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules: dav/fs/dbm.c
Posted by Colm MacCarthaigh <co...@stdlib.net>.
On Tue, Oct 04, 2005 at 08:11:46AM -0400, Jim Jagielski wrote:
> I do think the error message is wrong though :)
*bangs head off of table*
Thanks :)
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net
Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules:
dav/fs/dbm.c
Posted by André Malo <nd...@perlig.de>.
* Joe Orton <jo...@redhat.com> wrote:
> On Tue, Oct 04, 2005 at 11:03:31AM +0100, Colm MacCarthaigh wrote:
> > On Mon, Oct 03, 2005 at 08:38:02AM -0400, Jim Jagielski wrote:
> > > At the very last, if we are "assuming" behavior which is specifically
> > > implementation dependent, then a test during configure time that
> > > ensures sizeof(void *) <= sizeof(long) makes sense.
> >
> > > There is no room, IMO, for silent hidden assumptions in httpd.
> >
> > How about;
>
> -0.5, benefit is nil.
>
> I don't think it's a good idea to clutter up configure with checks for
> hypothetical platforms since the scope is unlimited and the build system
> is fragile enough already.
Then let's write valid ANSI C plus POSIX. I don't see a benefit in
writing fragile code as well.
(In this particular example I don't see a valid reason to store boolean
stuff in a pointer at all. This is just bad. But perhaps I'm overlooking
something.)
> We could go through this forever; "hey, will
> httpd work if sizeof(char) != 1?" "hmm, doubt it, lets add a configure
> check for that too" etc.
Huh? The standard specifies that sizeof(char) is always 1. So that is the
point to stop.
Just my EUR 0.02,
nd
Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules: dav/fs/dbm.c
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Oct 04, 2005 at 11:03:31AM +0100, Colm MacCarthaigh wrote:
> On Mon, Oct 03, 2005 at 08:38:02AM -0400, Jim Jagielski wrote:
> > At the very last, if we are "assuming" behavior which is specifically
> > implementation dependent, then a test during configure time that
> > ensures sizeof(void *) <= sizeof(long) makes sense.
>
> > There is no room, IMO, for silent hidden assumptions in httpd.
>
> How about;
-0.5, benefit is nil.
I don't think it's a good idea to clutter up configure with checks for
hypothetical platforms since the scope is unlimited and the build system
is fragile enough already. We could go through this forever; "hey, will
httpd work if sizeof(char) != 1?" "hmm, doubt it, lets add a configure
check for that too" etc.
joe
Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules: dav/fs/dbm.c
Posted by Colm MacCarthaigh <co...@stdlib.net>.
On Mon, Oct 03, 2005 at 08:38:02AM -0400, Jim Jagielski wrote:
> At the very last, if we are "assuming" behavior which is specifically
> implementation dependent, then a test during configure time that
> ensures sizeof(void *) <= sizeof(long) makes sense.
>
> There is no room, IMO, for silent hidden assumptions in httpd.
How about;
Index: configure.in
===================================================================
--- configure.in (revision 293579)
+++ configure.in (working copy)
@@ -32,6 +32,9 @@
dnl export expanded and relative configure argument variables
APACHE_EXPORT_ARGUMENTS
+dnl confirm that a void pointer is large enough to store a long integer
+APACHE_CHECK_VOID_PTR_LEN
+
dnl Save user-defined environment settings for later restoration
dnl
APR_SAVE_THE_ENVIRONMENT(CPPFLAGS)
Index: acinclude.m4
===================================================================
--- acinclude.m4 (revision 293579)
+++ acinclude.m4 (working copy)
@@ -570,3 +570,24 @@
undefine([ap_ckver_cvar])
undefine([ap_ckver_name])
])
+
+dnl
+dnl APACHE_CHECK_VOID_PTR_LEN
+dnl
+dnl Checks if the size of a void pointer is at least as big as a "long"
+dnl integer type.
+dnl
+AC_DEFUN([APACHE_CHECK_VOID_PTR_LEN], [
+
+AC_CACHE_CHECK([for void pointer length], [ap_void_ptr_lt_long],
+[AC_TRY_RUN([
+int main(void)
+{
+ return sizeof(void *) < sizeof(long);
+}], [ap_void_ptr_lt_long=yes], [ap_void_ptr_lt_long=no],
+ [ap_void_ptr_lt_long=no])])
+
+if test "$ap_void_ptr_lt_long" = "no"; then
+ AC_MSG_ERROR([Size of "void *" is less than size of "int"])
+fi
+])
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net
Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules:
dav/fs/dbm.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
> On Mon, Oct 03, 2005 at 08:11:44AM -0400, Jim Jagielski wrote:
>
>>Joe Orton wrote:
>>
>>>On Mon, Oct 03, 2005 at 07:50:39AM -0400, Jim Jagielski wrote:
>>>
>>>>Just some lines that caught my eye:
>>>>... Whenever I see conditionals cast to (long) I get
>>>>suspicious.
>>>
>>>These are all cases where an integer is stored in a pointer; it's safe
>>>to assume that a long will fit in a pointer on all platforms which httpd
>>>runs on as a practical consideration, and using a cast to long rather
>>>than a cast to int will avoid compiler warnings on LP64 platforms.
>>
>>... Certainly a union could be
>>used to avoid this.
>
> I usually end up deciding these issues are so marginal
> that there's some better way to spend time fixing real bugs than to try
> and break code which works perfectly well ;)
Joe's correct that this code change works on ILP32, ILP64 and LP64,
platforms - but I concur with Jim that for the casual developer, the
purpose is hard to glean...
...perhaps we need an apr_intptr_t type which is a best-fit int for any
arbitrary void* storage class?
Bill
Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules: dav/fs/dbm.c
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Oct 03, 2005 at 08:11:44AM -0400, Jim Jagielski wrote:
> Joe Orton wrote:
> >
> > On Mon, Oct 03, 2005 at 07:50:39AM -0400, Jim Jagielski wrote:
> > > Just some lines that caught my eye:
> > >
> > > >
> > > > - *context = (void *)(value == 'T');
> > > > + *context = (void *)((long)(value == 'T'));
> > > >
> > > > - int value = context != NULL;
> > > > + long value = context != NULL;
> > > > apr_fileperms_t perms = resource->info->finfo.protection;
> > > > - int old_value = (perms & APR_UEXECUTE) != 0;
> > > > + long old_value = (perms & APR_UEXECUTE) != 0;
> > > >
> > >
> > > Huh? Whenever I see conditionals cast to (long) I get
> > > suspicious.
> >
> > These are all cases where an integer is stored in a pointer; it's safe
> > to assume that a long will fit in a pointer on all platforms which httpd
> > runs on as a practical consideration, and using a cast to long rather
> > than a cast to int will avoid compiler warnings on LP64 platforms.
> >
>
> The above explanation worries me to no end. :) Certainly a union could be
> used to avoid this.
Well there are solutions, sure, not sure how a union would help here.
Try it and see... I usually end up deciding these issues are so marginal
that there's some better way to spend time fixing real bugs than to try
and break code which works perfectly well ;)
joe
Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules: dav/fs/dbm.c
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Oct 03, 2005 at 07:50:39AM -0400, Jim Jagielski wrote:
> Just some lines that caught my eye:
>
> >
> > - *context = (void *)(value == 'T');
> > + *context = (void *)((long)(value == 'T'));
> >
> > - int value = context != NULL;
> > + long value = context != NULL;
> > apr_fileperms_t perms = resource->info->finfo.protection;
> > - int old_value = (perms & APR_UEXECUTE) != 0;
> > + long old_value = (perms & APR_UEXECUTE) != 0;
> >
>
> Huh? Whenever I see conditionals cast to (long) I get
> suspicious.
These are all cases where an integer is stored in a pointer; it's safe
to assume that a long will fit in a pointer on all platforms which httpd
runs on as a practical consideration, and using a cast to long rather
than a cast to int will avoid compiler warnings on LP64 platforms.
joe