[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--