[MLton] Windows ports and paths

Wesley W. Terpstra wesley@terpstra.ca
Sun, 1 May 2005 13:29:31 +0200


--oyUTqETQ0mS9luUI
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sun, May 01, 2005 at 01:37:42AM -0700, Stephen Weeks wrote:
> Thanks for the patch.  I committed all the c99-->gnu99 changes.  I
> didn't commit the change from "-mcpu=pentiumpro" to
> "-march=pentiumpro", because I'm not sure what our position is on
> generating specialized code that doesn't run on some archs. 

Erp. That wasn't supposed to be in the diff. =)

I change that every time though because I get a warning on compiling the C
files that -mcpu is obsolete. This makes "make >/dev/null" still spam me
and I miss the important messages.

I would recommend just omitting -mtune=* altogether. -mtune=pentiumpro makes
things slower on modern CPUs in my experience. gcc's default seems to make a
good trade-off. Allowing gcc to use more instructions has some benefit tho.

> > The big problem I had is the question about what isAbs should return
> > for "\".
> 
> The basis library spec is clear that in the Windows world "\" is
> absolute.

Yeah, I suppose you're right; it does say this in several places.
However, I still don't like it. ;-)

> I realize that MinGW <> Windows, so there is some wiggle room.

I don't agree; I think MinGW is as native as it gets wrt windows.
I suppose VC++ is a tad "more native", but it also isn't free! =)

> > The second, slash_is_absolute.patch considers "\" to be absolute
> > and "E:\" simply the same as "\" with a volume.
> 
> I've checked in this one for now.  

This is probably the safer patch, though that's not the patch you commited!

What you commited is the ["", "foo"] patch. However, that patch was better
written anyways since it was built off of the other. I've prepared a
hand-made 3-way diff which backs out the changes that made it say "\" is not
absolute, but keep the other cleanups and fixes made since not_absolute.

I've check the dif to revision 1.11 from what you get after applying this
patch and it comes out to just an improved version of slash_is_absolute.
BTW, my definition of isArc sucks, if you have a better idiom for detecting
an isbad char, please change it. =)

> If we want to go the other way, it's worth discussing with the basis
> library designers and other SML implementors by sending an email to
> sml-basis-discuss@cs.uchicago.edu.

I don't really see how to make it work well. The [ "", "foo" ] approach 
is not satisfactory. I just don't like how 'absolute' paths can be changed
by chdir(). Perhaps a note that isAbs doesn't mean this and that isStable
(or something) means that: isAbs andalso (not isWindows or vol <> "").

> > I did a double-check of the patch under linux, and there is a small
> > regression in unixPath.sml, but I think the old behaviour was wrong:
> > joinDirFile {dir = "/c/a/b", file = ""} should be "/c/a/b" afaics.
> > -- the new concat was taken directly from the standard...
> 
> I agree that concat should do as you say, but I am not so sure for
> joinDirFile.  Looking at just the wording for joinDirFile in the spec,
> it's not clear what the right answer is.  However, looking also at
> splitDirFile shows us that
> 
> 	splitDirFile "b/" = {dir = "b", file = ""}
> 
> which fits well with
> 
> 	joinDirFile {dir = "b", file = ""} = "b/"
> 
> which makes it seem that the old behavior was correct.  I'll also
> point out that every SML implementation except for Hamlet does it like
> MLton.  Based on past experience, this probably means that every
> implementation except for Hamlet is wrong :-).  Andreas, any thoughts?

I defer to your expertise.

-- 
Wesley W. Terpstra

--oyUTqETQ0mS9luUI
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="threeway.diff"

Index: path.sml
===================================================================
RCS file: /cvsroot/mlton/mlton/basis-library/system/path.sml,v
retrieving revision 1.13
diff -u -r1.13 path.sml
--- path.sml	1 May 2005 08:44:33 -0000	1.13
+++ path.sml	1 May 2005 11:15:44 -0000
@@ -92,25 +92,6 @@
    * The big problem with windows paths is "\foo""
    * - It's not absolute, since chdir("A:\") may switch from "C:", thus
    *   changing the meaning of "\foo".
-   * - However, it's different from (and 'more absolute' than) "foo"
-   *
-   * Somehow, we need to distinguish "\foo" and "foo" without using isAbs
-   * like is done for Unix paths. Trying to keep the leading "\" in the
-   * arc leads to a mess of interactions later, so I don't do this.
-   * It seems to make the most sense to just allow a leading "" for
-   * non-absolute paths under windows. This has implications only in
-   * the implementation of mkCanonical, concat, and isRoot.
-   * 
-   * I propose for Windows:
-   * "E:foo"  => { isAbs=false, vol="E:", arcs=["foo"]  }
-   * "E:\foo" => { isAbs=true,  vol="E:", arcs=["foo"]  }
-   * "\foo"   => { isAbs=false, vol="",   arcs=["", "foo"] }
-   * "foo"    => { isAbs=false, vol="",   arcs=["foo"]  }
-   * "/foo"   => { isAbs=true,  vol="/",  arcs=["foo"]  } (cygwin volumeHack)
-   *
-   * For UNIX:
-   * "foo"    => { isAbs=false, vol="",   arcs=["foo"]  }
-   * "/foo"   => { isAbs=true,  vol="",   arcs=["foo"]  }
    *)
   fun validVolume {isAbs, vol} = 
      if isWindows 
@@ -130,10 +111,7 @@
         val (isAbs, arcs) =
            case (String.fields isslash rest) of
                 "" :: [] => (false, [])
-              | "" :: r => 
-                 if isWindows andalso vol = "" 
-                 then (false, "" :: r)
-                 else (true, r)
+              | "" :: r => (true, r)
               | r => (false, r)
      in
         {isAbs=isAbs, vol=vol, arcs=arcs}
@@ -158,8 +136,7 @@
   fun toString {arcs, isAbs, vol} =
      if not (validVolume {isAbs = isAbs, vol = vol})
 	then raise Path
-     else if not isWindows andalso not isAbs andalso 
-             case arcs of ("" :: _) => true | _ => false
+     else if not isAbs andalso case arcs of ("" :: _) => true | _ => false
         then raise Path
      else if List.exists (not o isArc) arcs
 	then raise InvalidArc
@@ -168,25 +145,18 @@
         (if isAbs andalso (not volumeHack orelse vol <> "/") then slash else "") ^ 
         String.concatWith slash arcs
 
-  (* The standard doesn't address:
-   *    concat("E:foo", "\foo") --> I say, raise Path
-   *)
   fun concat (p1, p2) =
      let
         fun cutEmptyTail l = 
            List.rev (case List.rev l of ("" :: r) => r | l => l)
         fun concatArcs (a1, []) = a1
           | concatArcs (a1, a2) = cutEmptyTail a1 @ a2
-        fun illegalJoin (_ :: _, "" :: _) = true
-          | illegalJoin _ = false
      in
         case (fromString p1, fromString p2) of
              (_, {isAbs=true, ...}) => raise Path
            | ({isAbs, vol=v1, arcs=a1}, {vol=v2, arcs=a2, ...}) =>
               if not (volumeMatch (v1, v2))
                  then raise Path
-              else if isWindows andalso illegalJoin (a1, a2)
-                 then raise Path
               else toString { isAbs=isAbs, vol=v1, arcs=concatArcs (a1, a2) }
      end
 
@@ -198,7 +168,6 @@
             | "." :: r => parentArc :: r
             | ".." :: r => parentArc :: parentArc :: r
             | _ :: [] => if isAbs then [""] else [currentArc]
-            | _ :: "" :: [] => ["", ""] (* \ *)
             | "" :: r => parentArc :: r
 	    | _ :: r => r)
       in
@@ -213,13 +182,9 @@
              then String.translate (str o Char.toLower) a
              else a
           
-          val driveTop = case arcs of "" :: _ => true | _ => false
-          val isRoot = isAbs orelse driveTop
-          val bump = if driveTop andalso not isAbs then [""] else []
-        
 	  fun backup l =
 	     case l of
-		[] => if isRoot then [] else [parentArc]
+		[] => if isAbs then [] else [parentArc]
 	      | first :: res =>
 		   if first = ".."
 		      then parentArc :: parentArc :: res
@@ -230,8 +195,8 @@
 		 fun h l res =
 		    case l of
 		       [] => (case res of
-				 [] => if isRoot then bump @ [""] else [currentArc]
-			       | _ => res @ bump)
+				 [] => if isAbs then [""] else [currentArc]
+			       | _ => res )
 		     | a1 :: ar =>
 			  if a1 = "" orelse a1 = "."
 			     then h ar res
@@ -246,11 +211,8 @@
   fun parentize []      = []
     | parentize (_::ar) = parentArc :: parentize ar
 
-  fun hackRoot {vol, arcs=""::r, ...} = {isAbs=true, vol=vol, arcs=r}
-    | hackRoot x = x
-  
   fun mkRelative {path = p1, relativeTo = p2} =
-      case (hackRoot (fromString p1), hackRoot (fromString (mkCanonical p2))) of
+      case (fromString p1, fromString (mkCanonical p2)) of
 	  (_ ,                {isAbs=false,...}) => raise Path
 	| ({isAbs=false,...}, _                ) => p1
 	| ({vol=vol1, arcs=arcs1,...}, {vol=vol2, arcs=arcs2, ...}) =>
@@ -319,7 +281,6 @@
   fun isRoot path =
      case fromString path of
 	{isAbs = true,  arcs=[""],  ...} => true
-      | {isAbs = false, arcs=["", ""], ...} => isWindows
       | _ => false
   
   fun fromUnixPath s = 

--oyUTqETQ0mS9luUI--