Hi all,
It'd be really nice if we could have a developer with windows look over
these patches, see what works, what doesn't, and generally give feedback.
I'm happy to review Ian's patches, but in this case I'm not particularly
competent to do so. It'd be great if we had a windows developer who was
willing to work with Ian on these changes.
David
On Sat, Jul 30, 2005 at 03:28:58PM +0100, Ian Lynagh wrote:
> Sat Jul 30 01:48:29 BST 2005 Ian Lynagh <[EMAIL PROTECTED]>
> * Start Compat.hs, and move stdout_is_a_pipe from compat.c
I hate to object to the very first patch, but I notice that our C code in
compat.c is compiled for win32 as well as unixy systems. So I presume that
the stdout_is_a_pipe C code compiles and works on windows systems, in which
case this patch is a regression, unless
import System.Posix.Files ( getFdStatus, isNamedPipe )
import System.Posix.IO ( stdOutput )
are available on windows (which I suspect they aren't). :( I'm not really
clear *what* is available on windows in terms of stuff like this (which I
wouldn't have expected to be there).
Maybe I'm wrong... I notice that in other places you import System.Posix.IO
within an #ifdef WIN32, so either you've got lots of windows problems, or
I'm being overly paranoid. Have you actually tried this stuff on windows?
If this really does work on windows, then the patch looks fine.
> Sat Jul 30 02:01:18 BST 2005 Ian Lynagh <[EMAIL PROTECTED]>
> * Remove unused function
Definitely safe. I actually tested, just to be sure... :)
> Sat Jul 30 03:09:18 BST 2005 Ian Lynagh <[EMAIL PROTECTED]>
> * Move mkstemp to Compat.hs
Do you think that maybe code like
import System.Posix.Files ( getFdStatus, isNamedPipe )
#ifdef WIN32
import System.Posix.Files ( stdFileMode )
#endif
would be clearer as
import System.Posix.Files ( getFdStatus, isNamedPipe,
#ifdef WIN32
stdFileMode,
#endif
)
This takes one more line, but makes it easier to see when reading that the
WIN32 case is just importing one more function from the same module. All
the #ifdefs in Compat.hs make me dizzy.
Other than that (assuming the code actually works on win32) it looks fine
to me.
> Sat Jul 30 13:22:55 BST 2005 Ian Lynagh <[EMAIL PROTECTED]>
> * Remove is_symlink
I think this looks good to me. If getSymbolicLinkStatus doesn't exist on
windows, we've still got the same number of #ifdefs as previously... :)
> Sat Jul 30 14:12:05 BST 2005 Ian Lynagh <[EMAIL PROTECTED]>
> * Move maybe_relink out of compat.c
...
> +#if defined(__GLASGOW_HASKELL__)
> + if use_mmap
> + then liftM MMappedPackedString (mmapFilePS f)
> + else
> +#endif
I was wondering why the #if defined is in there. I can see that we only
use mmap with ghc. Is this because the ForeignPtr support in hugs isn't
capable of dealing with our unmap properly, or what?
> hunk ./FastPackedString.hs 819
> + deriving Eq
I'd rather have a comment here pointing out that this Eq isn't really
correct. You have a comment in maybe_relink to that effect, but it'd be
nice eventually to make it a truly correct ==. Having functions that don't
behave as one would expect scares me.
> Sat Jul 30 14:40:30 BST 2005 Ian Lynagh <[EMAIL PROTECTED]>
> * Split the raw mode stuff out into its own .hsc file. Windows needs some
> TLC
Hmmm. Do we even want to compile this thing if we aren't running windows?
It would seem nicer to put the #ifdefs in the calling function,
without_buffering.
> Sat Jul 30 15:17:03 BST 2005 Ian Lynagh <[EMAIL PROTECTED]>
> * Move atomic_create/sloppy_atomic_create to Compat
Looks all right (but I'm just skimming at this stage).
--
David Roundy
http://www.darcs.net
_______________________________________________
darcs-devel mailing list
[email protected]
http://www.abridgegame.org/cgi-bin/mailman/listinfo/darcs-devel
