You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Greg Stein <gs...@lyra.org> on 2000/12/07 09:08:56 UTC
Re: cvs commit: apr/mmap/win32 mmap.c
On Thu, Dec 07, 2000 at 06:59:00AM -0000, wrowe@locus.apache.org wrote:
> wrowe 00/12/06 22:58:59
>
> Modified: . CHANGES aprlib.dsp
> include apr.hw apr_mmap.h
> Added: mmap/win32 mmap.c
> Log:
> Implement Win32 MMAP support.
>...
> --- apr_mmap.h 2000/12/07 05:00:27 1.18
> +++ apr_mmap.h 2000/12/07 06:58:59 1.19
> @@ -90,6 +90,16 @@
> /** An area ID. Only valid on BeOS */
> area_id area;
> #endif
> +#ifdef WIN32
> + /** The handle of the file mapping */
> + HANDLE mhandle;
> + /** The start of the real memory page area (mapped view) */
> + void *mv;
> + /** The physical start, size and offset */
> + size_t pstart;
> + size_t psize;
> + size_t poffset;
> +#endif
Non-win32-specific question: why is the MMAP structure visible? Shouldn't
that be an opaque structure?
>...
> #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...
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
RE: cvs commit: apr/mmap/win32 mmap.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
> From: Greg Stein [mailto:gstein@lyra.org]
> Sent: Thursday, December 07, 2000 7:52 AM
>
> On Thu, Dec 07, 2000 at 07:33:31AM -0600, William A. Rowe, Jr. wrote:
> > rbb and I agreed last night,
>
> Agreed *what* ?
with you, that apr_mmap_t aught to go incomplete :-)
> > 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.
I don't have a huge issue with it.
> > We left it for you to find :-)
>
> Find what? I'm lost.
the (inappropriately) complete apr_mmap_t type.
Making it incomplete needed to be it's own patch, anyways.
> Doing the [#if APR_HAS_MMAP in win32/mmap.c] is confusing because
> (as I mentioned) it seems to imply that APR_HAS_MMAP might be zero
> in some (unknown) situation.
And maybe it will be ... when I've build and tested on a 9x platform.
Don't know yet that it will work. I'm not saying _I_ want to make my
inactive, I'm saying if others [Alan, Bill, Jeff, whomever] find a
quirck, they can work around it. That's how I coded APR_HAS_UNICODE_FS,
since it's a radically new experiment [which you and I still need to
hammer out, but please save discussion for the afternoon :-]
> 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. :-)
On this sort of thing, fine. I'll apply a bit later. {I'm leaving alone
the 'other' discussion for this morning, too much on my plate. Patch will
preceed the canonfilename Apache stuff.)
Re: cvs commit: apr/mmap/win32 mmap.c
Posted by Greg Stein <gs...@lyra.org>.
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/
Re: cvs commit: apr/mmap/win32 mmap.c
Posted by rb...@covalent.net.
> > +#ifdef WIN32
> > + /** The handle of the file mapping */
> > + HANDLE mhandle;
> > + /** The start of the real memory page area (mapped view) */
> > + void *mv;
> > + /** The physical start, size and offset */
> > + size_t pstart;
> > + size_t psize;
> > + size_t poffset;
> > +#endif
>
> Non-win32-specific question: why is the MMAP structure visible? Shouldn't
> that be an opaque structure?
It was to begin with, then I started adding MMAP support to 2.0, and
making it a visible structure made adding it to Apache much easier, and it
just made sense. Since I added it, the code has changed, and we won't
have any problems hiding this structure again. Will and I discussed this
last night, and I'll probably end up making this structure incomplete
today if I have a chance.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
RE: cvs commit: apr/mmap/win32 mmap.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
> From: Greg Stein [mailto:gstein@lyra.org]
> Sent: Thursday, December 07, 2000 2:09 AM
>
> > wrowe 00/12/06 22:58:59
> >
> > --- apr_mmap.h 2000/12/07 05:00:27 1.18
> > +++ apr_mmap.h 2000/12/07 06:58:59 1.19
> > @@ -90,6 +90,16 @@
> > /** An area ID. Only valid on BeOS */
> > area_id area;
> > #endif
> > +#ifdef WIN32
> > + /** The handle of the file mapping */
> > + HANDLE mhandle;
> > + /** The start of the real memory page area (mapped view) */
> > + void *mv;
> > + /** The physical start, size and offset */
> > + size_t pstart;
> > + size_t psize;
> > + size_t poffset;
> > +#endif
>
> Non-win32-specific question: why is the MMAP structure
> visible? Shouldn't that be an opaque structure?
rbb and I agreed last night, except that there are some optimizations
that go away with hiding the type (contrawise, common.c code should be
macros, if this type remains visible.) We left it for you to find :-)
> >...
> > #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. 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.
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.