apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: cvs commit: apr/mmap/win32 mmap.c
Date Thu, 07 Dec 2000 13:52:23 GMT
On Thu, Dec 07, 2000 at 07:33:31AM -0600, William A. Rowe, Jr. wrote:
> > From: Greg Stein [mailto:gstein@lyra.org]
> > Sent: Thursday, December 07, 2000 2:09 AM
>...
> > Non-win32-specific question: why is the MMAP structure 
> > visible? Shouldn't that be an opaque structure?

Dude. You're being obscure below...

> rbb and I agreed last night,

Agreed *what* ?

> except that there are some optimizations
> that go away with hiding the type

What kind of optimizations? Are we talking a few cycles? Let's go with the
hiding and take the hit of a few cycles.

> (contrawise, common.c code should be
> macros, if this type remains visible.)  We left it for you to find :-)

Find what? I'm lost.

> > >...
> > >   #include "apr.h"
> > >   #include "apr_private.h"
> > >   #include "apr_general.h"
> > >   #include "apr_mmap.h"
> > >   #include "apr_errno.h"
> > >   #include "fileio.h"
> > >   #include "apr_portable.h"
> > >   
> > >   #if APR_HAS_MMAP
> > 
> > Why put this in here? apr_private.hw defines it to 1. The above statement
> > seems to imply that sometimes it will be non-zero. And that just isn't the
> > case...
> 
> Because I always leave the option to -quickly- yank new code.

Add a #if 0 if you want to yank it. Check in a reverse patch. Delete the
file. Whatever.

Doing the above is confusing because (as I mentioned) it seems to imply that
APR_HAS_MMAP might be zero in some (unknown) situation.

> The new
> filename canonical will go into the server first before Sunday night, and
> will retain (for now) a bunch of extra cruft in the non-canonical compilation
> path.

1) post a patch first. I know there is still a good amount of "what the
   hell" going on, even after the hackathon.

2) screw leaving the cruft. we can always reverse patch. leaving the old
   code will just confuse matters, yet we'll be trying to review the new
   stuff.

Point is (same as above): make CVS work for you... don't add complexity to
the code to replicate what CVS can already do. Don't add "#if APR_HAS_MMAP"
on the *chance* that you might want to back it out; do something if/when you
need to back it.

> This test could go away whenever.  The other reason it's left is to remember 
> that it isn't tested on all win platforms, and we know that the Win32 API
> discrepancies are worse than five releases of three different ports of Unix.

Leave a comment rather than code structure.

/* ### this hasn't been tested on all Win32 systems */

That is a LOT clearer than "#if APR_HAS_MMAP". The #if says nothing about
testing on other systems. If that was your intent, then say so. :-)

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

Mime
View raw message