On Thu, Dec 07, 2000 at 07:33:31AM -0600, William A. Rowe, Jr. wrote: > > From: Greg Stein [mailto:[EMAIL PROTECTED] > > 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/
