[MLton] Patch ready
Matthew Fluet
fluet at tti-c.org
Sun Feb 11 16:26:52 PST 2007
The SSA type-checking bug is fixed and we'll punt on the exposure of the
string = CharVector.vector = char vector equivalence.
Here are some notes on the WideChar patch. There's nothing too major,
so feel free to commit after addressing the comments.
-------------- next part --------------
Index: regression/widechar.sml
===================================================================
--- regression/widechar.sml (revision 0)
+++ regression/widechar.sml (revision 0)
Looks like a port of bytechar.sml; shouldn't return "WRONG".
Index: mlton/control/control-flags.sig
===================================================================
--- mlton/control/control-flags.sig (revision 5163)
+++ mlton/control/control-flags.sig (working copy)
Index: mlton/control/control-flags.sml
===================================================================
--- mlton/control/control-flags.sml (revision 5163)
+++ mlton/control/control-flags.sml (working copy)
Index: mlton/main/main.fun
===================================================================
--- mlton/main/main.fun (revision 5163)
+++ mlton/main/main.fun (working copy)
These all look good; I'm not sure that we'll ever really need to
exercise the choice of WideChar binding (indeed, if we interpret it
like LargeInt, then it is pretty much dicated to be Char32), but it is
nice to have it.
Index: mlton/atoms/word-x-vector.fun
===================================================================
--- mlton/atoms/word-x-vector.fun (revision 5163)
+++ mlton/atoms/word-x-vector.fun (working copy)
@@ -39,7 +39,11 @@
Char.fromInt (IntInf.toInt r) :: ac)
end
in
- loop (n, WordX.toIntInf w, ac)
+ (* Control.targetIsBigEndian is not always set, so
+ * only use it if we really need to know the value. *)
+ if n > 8 andalso Control.targetIsBigEndian ()
+ then rev (loop (n, WordX.toIntInf w, [])) @ ac
+ else loop (n, WordX.toIntInf w, []) @ ac
end)))
end
This endian handling is probably best handled by the C-codegen. As
you point out, the target endianness isn't always set and it would be
nice if the IL (and their printed representations) were target
agnostic. Also, turning a WordXVector.t into a string is used for
hashing, so making it target endian dependent changes hash values.
But, in the meantime, this seems expedient.
Index: basis-library/arrays-and-vectors/vector-slice.sig
===================================================================
--- basis-library/arrays-and-vectors/vector-slice.sig (revision 5163)
+++ basis-library/arrays-and-vectors/vector-slice.sig (working copy)
Index: basis-library/arrays-and-vectors/slice.sig
===================================================================
--- basis-library/arrays-and-vectors/slice.sig (revision 5163)
+++ basis-library/arrays-and-vectors/slice.sig (working copy)
@@ -86,7 +86,7 @@
* ('a sequence * 'a sequence -> bool) should be polymorphic equality
*)
val span: ('a sequence * 'a sequence -> bool) -> 'a slice * 'a slice -> 'a slice
- val translate: ('a elt -> 'a sequence) -> 'a slice -> 'a sequence
+ val translate: ('a elt -> 'b sequence) -> 'a slice -> 'b sequence
val tokens: ('a elt -> bool) -> 'a slice -> 'a slice list
val fields: ('a elt -> bool) -> 'a slice -> 'a slice list
Index: basis-library/arrays-and-vectors/vector.sig
===================================================================
--- basis-library/arrays-and-vectors/vector.sig (revision 5163)
+++ basis-library/arrays-and-vectors/vector.sig (working copy)
Index: basis-library/arrays-and-vectors/sequence.sig
===================================================================
--- basis-library/arrays-and-vectors/sequence.sig (revision 5163)
+++ basis-library/arrays-and-vectors/sequence.sig (working copy)
@@ -64,7 +64,7 @@
val isPrefix: ('a elt * 'a elt -> bool) -> 'a sequence -> 'a sequence -> bool
val isSubsequence: ('a elt * 'a elt -> bool) -> 'a sequence -> 'a sequence -> bool
val isSuffix: ('a elt * 'a elt -> bool) -> 'a sequence -> 'a sequence -> bool
- val translate: ('a elt -> 'a sequence) -> 'a sequence -> 'a sequence
+ val translate: ('a elt -> 'b sequence) -> 'a sequence -> 'b sequence
val tokens: ('a elt -> bool) -> 'a sequence -> 'a sequence list
val fields: ('a elt -> bool) -> 'a sequence -> 'a sequence list
All of the sequence/slice/vector/vector-slice functions are there to
implement String and Substring functions at a place where we're more
free with unsafe subscript operations. String.translate and
Substring.translate don't need the extra polymorphism, so I'm guessing
that you need it somewhere. In any case, the extra polymorphism
doesn't hurt.
Index: basis-library/arrays-and-vectors/mono.sml
===================================================================
--- basis-library/arrays-and-vectors/mono.sml (revision 5163)
+++ basis-library/arrays-and-vectors/mono.sml (working copy)
Good.
Index: basis-library/text/algo.sml
===================================================================
--- basis-library/text/algo.sml (revision 0)
+++ basis-library/text/algo.sml (revision 0)
Please move this to basis-library/util/. Also, please give it a
containing structure; in the Basis Library implementation, we try to
avoid top-level values that are not part of the top-level environment
of the Basis Library. (Although that signature match with
BASIS_{EXTRA,2002} generally culls any extra bindings, its a good
convention to follow.)
Index: basis-library/config/default/default-widechar32.sml
===================================================================
--- basis-library/config/default/default-widechar32.sml (revision 0)
+++ basis-library/config/default/default-widechar32.sml (revision 0)
Index: basis-library/config/default/default-widechar16.sml
===================================================================
--- basis-library/config/default/default-widechar16.sml (revision 0)
+++ basis-library/config/default/default-widechar16.sml (revision 0)
Good.
One thing I notice about the design is that you don't try to propagate
CharX and StringX structures through everything; rather, after
choosing Char and WideChar using
basis/library/config/default/default-{charN,widecharN}.sml, everything
is expressed in terms of Char and WideChar. This seems good, and it
is somewhat simpler than what needs to happen with IntN and WordN.
Index: basis-library/text/string0.sml
===================================================================
--- basis-library/text/string0.sml (revision 5163)
+++ basis-library/text/string0.sml (working copy)
Give a PRE_STRING signature? Also, PreWideString seems unused.
Index: basis-library/text/char0.sml
===================================================================
--- basis-library/text/char0.sml (revision 5163)
+++ basis-library/text/char0.sml (working copy)
Looks good. You could give a PRE_CHAR signature. You could also use
a local .. in .. end to not expose the PreCharX structure. (Nice use
of the {Char,WideChar}_ChooseChar functors.)
Index: basis-library/text/string-cvt.sig
===================================================================
--- basis-library/text/string-cvt.sig (revision 5163)
+++ basis-library/text/string-cvt.sig (working copy)
Index: basis-library/text/string-cvt.sml
===================================================================
--- basis-library/text/string-cvt.sml (revision 5163)
+++ basis-library/text/string-cvt.sml (working copy)
Fine; I see that it would be a bit hard to implement all of the is*
predicates before having the numeric structures around.
Index: basis-library/integer/int-inf.sml
===================================================================
--- basis-library/integer/int-inf.sml (revision 5163)
+++ basis-library/integer/int-inf.sml (working copy)
Great.
Index: basis-library/text/char.sig
===================================================================
--- basis-library/text/char.sig (revision 5163)
+++ basis-library/text/char.sig (working copy)
@@ -38,17 +38,18 @@
val isPrint: char -> bool
val isPunct: char -> bool
val isSpace: char -> bool
- val fromString: string -> char option
- val scan: (char, 'a) StringCvt.reader -> (char, 'a) StringCvt.reader
- val toString: char -> string
- val fromCString: string -> char option
- val toCString: char -> string
+
+ val toString: char -> String.string
+ val scan: (Char.char, 'a) StringCvt.reader -> (char, 'a) StringCvt.reader
+ val fromString: String.string -> char option
+ val toCString: char -> String.string
+ val fromCString: String.string -> char option
end
signature CHAR_EXTRA =
sig
include CHAR
- val formatSequences: (char, 'a) StringCvt.reader -> 'a -> 'a
- val scanC: (char, 'a) StringCvt.reader -> (char, 'a) StringCvt.reader
+ val formatSequences: (Char.char, 'a) StringCvt.reader -> 'a -> 'a
+ val scanC: (Char.char, 'a) StringCvt.reader -> (char, 'a) StringCvt.reader
end
Good; that fixes a minor mismatch with the Basis specification.
Index: basis-library/text/string.sig
===================================================================
--- basis-library/text/string.sig (revision 5163)
+++ basis-library/text/string.sig (working copy)
@@ -33,7 +33,7 @@
val isSuffix: string -> string -> bool
val map: (char -> char) -> string -> string
val maxSize: int
- val scan: (char, 'a) StringCvt.reader -> (string, 'a) StringCvt.reader
+ val scan: (Char.char, 'a) StringCvt.reader -> (string, 'a) StringCvt.reader
val sub: string * int -> char
val toCString: string -> String.string
val toString: string -> String.string
This seems to go against the Basis specification. The STRING.scan
function is supposed to be in terms of the STRING.char type. But, it
is an odd type given the type of the corresponding function in CHAR,
so I think you are right in spirit.
Index: basis-library/text/char.sml
===================================================================
--- basis-library/text/char.sml (revision 5163)
+++ basis-library/text/char.sml (working copy)
@@ -6,23 +6,156 @@
* See the file MLton-LICENSE for details.
*)
+ structure PreChar:
+ sig
...
+ end
Definitely worth making a distinguished PRE_CHAR signature.
+ val c = fromChar
+ val ( la, lA, lf, lF, lz, lZ, l0, l9, ls, lBANG, lTIL, lDEL) =
+ (c#"a", c#"A", c#"f", c#"F", c#"z", c#"Z", c#"0", c#"9", c#" ", c#"!", c#"~", c#"\127")
Use 'lSPACE' for consistency with BANG, TIL, and DEL?
But, otherwise, looks really great.
Index: basis-library/text/string.sml
===================================================================
--- basis-library/text/string.sml (revision 5163)
+++ basis-library/text/string.sml (working copy)
Great.
Index: basis-library/text/substring.sml
===================================================================
--- basis-library/text/substring.sml (revision 5163)
+++ basis-library/text/substring.sml (working copy)
- fun substring (s, start, len) = extract (s, start, SOME len)
+ fun substring (s, start, len) = extract (s, start, SOME len)
Extraneous white space.
Index: basis-library/text/text.sml
===================================================================
--- basis-library/text/text.sml (revision 5163)
+++ basis-library/text/text.sml (working copy)
Good.
Index: basis-library/libs/basis-2002/top-level/basis.sig
===================================================================
--- basis-library/libs/basis-2002/top-level/basis.sig (revision 5163)
+++ basis-library/libs/basis-2002/top-level/basis.sig (working copy)
Index: basis-library/libs/basis-extra/top-level/basis.sig
===================================================================
--- basis-library/libs/basis-extra/top-level/basis.sig (revision 5163)
+++ basis-library/libs/basis-extra/top-level/basis.sig (working copy)
Index: basis-library/libs/basis-extra/top-level/basis.sml
===================================================================
--- basis-library/libs/basis-extra/top-level/basis.sml (revision 5163)
+++ basis-library/libs/basis-extra/top-level/basis.sml (working copy)
@@ -175,7 +175,6 @@
Good.
Index: basis-library/build/sources.mlb
===================================================================
--- basis-library/build/sources.mlb (revision 5163)
+++ basis-library/build/sources.mlb (working copy)
Good.
More information about the MLton
mailing list