From 92680733ef4001f908e80f36d9fea03cd9b08b4d Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Wed, 17 Jun 2020 03:42:16 +0200 Subject: [PATCH 01/10] Simplify and improve perf of String.length --- FSharp.v3.ncrunchsolution.user | 5 +++++ src/fsharp/FSharp.Core/string.fs | 6 ++---- 2 files changed, 7 insertions(+), 4 deletions(-) create mode 100644 FSharp.v3.ncrunchsolution.user diff --git a/FSharp.v3.ncrunchsolution.user b/FSharp.v3.ncrunchsolution.user new file mode 100644 index 00000000000..b82f7a534f2 --- /dev/null +++ b/FSharp.v3.ncrunchsolution.user @@ -0,0 +1,5 @@ + + + Run all tests automatically [Global] + + \ No newline at end of file diff --git a/src/fsharp/FSharp.Core/string.fs b/src/fsharp/FSharp.Core/string.fs index e620e4cac6a..5da167dafc0 100644 --- a/src/fsharp/FSharp.Core/string.fs +++ b/src/fsharp/FSharp.Core/string.fs @@ -104,7 +104,5 @@ namespace Microsoft.FSharp.Core [] let length (str:string) = - if String.IsNullOrEmpty str then - 0 - else - str.Length + if isNull str then 0 + else str.Length From 890ed1ff1427b92d03169ff8c341b234f28a6357 Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Wed, 17 Jun 2020 04:09:32 +0200 Subject: [PATCH 02/10] Improve performance of String.map --- FSharp.v3.ncrunchsolution.user | 5 ----- src/fsharp/FSharp.Core/string.fs | 13 ++++++++----- 2 files changed, 8 insertions(+), 10 deletions(-) delete mode 100644 FSharp.v3.ncrunchsolution.user diff --git a/FSharp.v3.ncrunchsolution.user b/FSharp.v3.ncrunchsolution.user deleted file mode 100644 index b82f7a534f2..00000000000 --- a/FSharp.v3.ncrunchsolution.user +++ /dev/null @@ -1,5 +0,0 @@ - - - Run all tests automatically [Global] - - \ No newline at end of file diff --git a/src/fsharp/FSharp.Core/string.fs b/src/fsharp/FSharp.Core/string.fs index 5da167dafc0..54a57ddcb17 100644 --- a/src/fsharp/FSharp.Core/string.fs +++ b/src/fsharp/FSharp.Core/string.fs @@ -31,12 +31,15 @@ namespace Microsoft.FSharp.Core [] let map (mapping: char -> char) (str:string) = - if String.IsNullOrEmpty str then - String.Empty + if String.IsNullOrEmpty str then String.Empty else - let res = StringBuilder str.Length - str |> iter (fun c -> res.Append(mapping c) |> ignore) - res.ToString() + let result = str.ToCharArray() + let mutable i = 0 + for c in result do + result.[i] <- mapping c + i <- i + 1 + + String result [] let mapi (mapping: int -> char -> char) (str:string) = From 10e583320f232435d10f5c986e91a431c722447a Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Wed, 17 Jun 2020 14:37:57 +0200 Subject: [PATCH 03/10] Revert "Simplify and improve perf of String.length" --- src/fsharp/FSharp.Core/string.fs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/fsharp/FSharp.Core/string.fs b/src/fsharp/FSharp.Core/string.fs index 54a57ddcb17..005abcc0cca 100644 --- a/src/fsharp/FSharp.Core/string.fs +++ b/src/fsharp/FSharp.Core/string.fs @@ -107,5 +107,7 @@ namespace Microsoft.FSharp.Core [] let length (str:string) = - if isNull str then 0 - else str.Length + if String.IsNullOrEmpty str then + 0 + else + str.Length From 6c6b6721c522bb0ceca0ea22142396178ed6e7f3 Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Wed, 17 Jun 2020 15:31:44 +0200 Subject: [PATCH 04/10] Resolves https://github.com/dotnet/fsharp/pull/9470#discussion_r441293049 --- src/fsharp/FSharp.Core/string.fs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/fsharp/FSharp.Core/string.fs b/src/fsharp/FSharp.Core/string.fs index 005abcc0cca..c5d6fc68980 100644 --- a/src/fsharp/FSharp.Core/string.fs +++ b/src/fsharp/FSharp.Core/string.fs @@ -31,7 +31,8 @@ namespace Microsoft.FSharp.Core [] let map (mapping: char -> char) (str:string) = - if String.IsNullOrEmpty str then String.Empty + if String.IsNullOrEmpty str then + String.Empty else let result = str.ToCharArray() let mutable i = 0 From c054bb2748af79516b000b3d28c65c9e6443246b Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Wed, 17 Jun 2020 15:45:14 +0200 Subject: [PATCH 05/10] Lingering space --- src/fsharp/FSharp.Core/string.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fsharp/FSharp.Core/string.fs b/src/fsharp/FSharp.Core/string.fs index c5d6fc68980..90afc1a0026 100644 --- a/src/fsharp/FSharp.Core/string.fs +++ b/src/fsharp/FSharp.Core/string.fs @@ -31,7 +31,7 @@ namespace Microsoft.FSharp.Core [] let map (mapping: char -> char) (str:string) = - if String.IsNullOrEmpty str then + if String.IsNullOrEmpty str then String.Empty else let result = str.ToCharArray() From e2cde1b2fd517a72f0a77efbe7846827f2637b3c Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Wed, 17 Jun 2020 15:59:12 +0200 Subject: [PATCH 06/10] Change `String` to use `new` to clarify use of ctor --- src/fsharp/FSharp.Core/string.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fsharp/FSharp.Core/string.fs b/src/fsharp/FSharp.Core/string.fs index 90afc1a0026..93d0e34ea66 100644 --- a/src/fsharp/FSharp.Core/string.fs +++ b/src/fsharp/FSharp.Core/string.fs @@ -40,7 +40,7 @@ namespace Microsoft.FSharp.Core result.[i] <- mapping c i <- i + 1 - String result + new String(result) [] let mapi (mapping: int -> char -> char) (str:string) = From 332420eb6d5d2172115f895458f65b9fac65e8d7 Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Thu, 18 Jun 2020 17:12:46 +0200 Subject: [PATCH 07/10] Add some better tests for String.map, add side-effect test --- .../StringModule.fs | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs index a72ab64e236..5372e45e09b 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs @@ -71,11 +71,26 @@ type StringModule() = [] member this.Map() = - let e1 = String.map (fun c -> c) "foo" + let e1 = String.map id "foo" Assert.AreEqual("foo", e1) - let e2 = String.map (fun c -> c) null - Assert.AreEqual("", e2) + let e2 = String.map (fun c -> c + char 1) "abcde" + Assert.AreEqual("bcdef", e2) + + let e3 = String.map (fun c -> c) null + Assert.AreEqual("", e3) + + let e4 = String.map (fun c -> c) String.Empty + Assert.AreEqual("", e4) + + let e5 = String.map (fun _ -> 'B') "A" + Assert.AreEqual("B", e5) + + // side-effect and "order of operation" test + let mutable x = 0 + let e6 = String.map (fun c -> x <- x + 1; c + char x) "abcde" + Assert.AreEqual(x, 5) + Assert.AreEqual(e6, "bdfhj") [] member this.MapI() = From 1b9bf8560f4fc2f77539238915351079a56833c3 Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Thu, 18 Jun 2020 17:25:06 +0200 Subject: [PATCH 08/10] Add tests to ensure the mapping function is called a deterministically amount of times --- .../Microsoft.FSharp.Collections/StringModule.fs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs index 5372e45e09b..1a047131dbf 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs @@ -86,11 +86,20 @@ type StringModule() = let e5 = String.map (fun _ -> 'B') "A" Assert.AreEqual("B", e5) + let e6 = String.map (fun _ -> failwith "should not raise") null + Assert.AreEqual("", e6) + + // this tests makes sure mapping function is not called too many times + let mutable x = 0 + let e7 = String.map (fun _ -> if x > 2 then failwith "should not raise" else x <- x + 1; 'x') "abc" + Assert.AreEqual(e7, 3) + Assert.AreEqual(e7, "xxx") + // side-effect and "order of operation" test let mutable x = 0 - let e6 = String.map (fun c -> x <- x + 1; c + char x) "abcde" + let e8 = String.map (fun c -> x <- x + 1; c + char x) "abcde" Assert.AreEqual(x, 5) - Assert.AreEqual(e6, "bdfhj") + Assert.AreEqual(e8, "bdfhj") [] member this.MapI() = From 0dd89227e3ff0693e45250131b3b2889207b279b Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Thu, 18 Jun 2020 17:30:37 +0200 Subject: [PATCH 09/10] Fix typo --- .../FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs index 1a047131dbf..b8421c0e9c5 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs @@ -92,7 +92,7 @@ type StringModule() = // this tests makes sure mapping function is not called too many times let mutable x = 0 let e7 = String.map (fun _ -> if x > 2 then failwith "should not raise" else x <- x + 1; 'x') "abc" - Assert.AreEqual(e7, 3) + Assert.AreEqual(x, 3) Assert.AreEqual(e7, "xxx") // side-effect and "order of operation" test From 83ac13da612d87a746f6807c0409fcd5de722ba2 Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Fri, 19 Jun 2020 18:49:25 +0200 Subject: [PATCH 10/10] Remove "foo" from String.map tests --- .../FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs index b8421c0e9c5..3542e9507a9 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/StringModule.fs @@ -71,8 +71,8 @@ type StringModule() = [] member this.Map() = - let e1 = String.map id "foo" - Assert.AreEqual("foo", e1) + let e1 = String.map id "xyz" + Assert.AreEqual("xyz", e1) let e2 = String.map (fun c -> c + char 1) "abcde" Assert.AreEqual("bcdef", e2)