From c777c6a85aaa27bd11b7128415ff08e018bc166d Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Thu, 18 Jun 2020 01:29:26 +0200 Subject: [PATCH 1/5] Move `String.length` to the top of its module so that the `length` function is in scope --- src/fsharp/FSharp.Core/string.fs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/fsharp/FSharp.Core/string.fs b/src/fsharp/FSharp.Core/string.fs index df51fccd031..7e4b65f1a08 100644 --- a/src/fsharp/FSharp.Core/string.fs +++ b/src/fsharp/FSharp.Core/string.fs @@ -12,6 +12,9 @@ namespace Microsoft.FSharp.Core [] [] module String = + [] + let length (str:string) = if isNull str then 0 else str.Length + [] let concat sep (strings : seq) = String.Join(sep, strings) @@ -101,6 +104,3 @@ namespace Microsoft.FSharp.Core else let rec check i = (i < str.Length) && (predicate str.[i] || check (i+1)) check 0 - - [] - let length (str:string) = if isNull str then 0 else str.Length From 53a235efeb3e5d22f576bcb3272a1d27da4e4eff Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Thu, 18 Jun 2020 02:15:43 +0200 Subject: [PATCH 2/5] Improve performance of String.concat for arrays and lists --- src/fsharp/FSharp.Core/string.fs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/fsharp/FSharp.Core/string.fs b/src/fsharp/FSharp.Core/string.fs index 7e4b65f1a08..0e024ea292f 100644 --- a/src/fsharp/FSharp.Core/string.fs +++ b/src/fsharp/FSharp.Core/string.fs @@ -8,6 +8,7 @@ namespace Microsoft.FSharp.Core open Microsoft.FSharp.Core.Operators open Microsoft.FSharp.Core.Operators.Checked open Microsoft.FSharp.Collections + open Microsoft.FSharp.Primitives.Basics [] [] @@ -15,9 +16,26 @@ namespace Microsoft.FSharp.Core [] let length (str:string) = if isNull str then 0 else str.Length + let private concatArray sep (strings: string []) = + match length sep with + | 0 -> String.Concat strings + // following line should be used when this overload becomes part of .NET Standard (it's only in .NET Core) + //| 1 -> String.Join(sep.[0], strings, 0, strings.Length) + | _ -> String.Join(sep, strings, 0, strings.Length) + [] let concat sep (strings : seq) = - String.Join(sep, strings) + match strings with + | :? array as arr -> + concatArray sep arr + + | :? list as lst -> + lst + |> List.toArray + |> concatArray sep + + | _ -> + String.Join(sep, strings) [] let iter (action : (char -> unit)) (str:string) = From d7c95e00055b35484c7d2ae6f789fd3a5fb0f138 Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Thu, 18 Jun 2020 17:50:36 +0200 Subject: [PATCH 3/5] Make concatArray local to String.concat --- src/fsharp/FSharp.Core/string.fs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/fsharp/FSharp.Core/string.fs b/src/fsharp/FSharp.Core/string.fs index 0e024ea292f..7f9b0a6e709 100644 --- a/src/fsharp/FSharp.Core/string.fs +++ b/src/fsharp/FSharp.Core/string.fs @@ -16,15 +16,16 @@ namespace Microsoft.FSharp.Core [] let length (str:string) = if isNull str then 0 else str.Length - let private concatArray sep (strings: string []) = - match length sep with - | 0 -> String.Concat strings - // following line should be used when this overload becomes part of .NET Standard (it's only in .NET Core) - //| 1 -> String.Join(sep.[0], strings, 0, strings.Length) - | _ -> String.Join(sep, strings, 0, strings.Length) - [] let concat sep (strings : seq) = + + let concatArray sep (strings: string []) = + match length sep with + | 0 -> String.Concat strings + // following line should be used when this overload becomes part of .NET Standard (it's only in .NET Core) + //| 1 -> String.Join(sep.[0], strings, 0, strings.Length) + | _ -> String.Join(sep, strings, 0, strings.Length) + match strings with | :? array as arr -> concatArray sep arr From 9c6ba3fe922e7677695dbbaf1ccc30271d3f4a6b Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Thu, 18 Jun 2020 18:09:31 +0200 Subject: [PATCH 4/5] Testing String.concat, make sure the new three paths are covered --- .../StringModule.fs | 44 +++++++++---------- 1 file changed, 20 insertions(+), 24 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..b71273d9c39 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 @@ -23,30 +23,26 @@ type StringModule() = [] member this.Concat() = - let e1 = String.concat null ["foo"] - Assert.AreEqual("foo", e1) - - let e2 = String.concat "" [] - Assert.AreEqual("", e2) - - let e3 = String.concat "foo" [] - Assert.AreEqual("", e3) - - let e4 = String.concat "" [null] - Assert.AreEqual("", e4) - - let e5 = String.concat "" [""] - Assert.AreEqual("", e5) - - let e6 = String.concat "foo" ["bar"] - Assert.AreEqual("bar", e6) - - let e7 = String.concat "foo" ["bav";"baz"] - Assert.AreEqual("bavfoobaz", e7) - - let e8 = String.concat "foo" [null;"baz";null;"bar"] - Assert.AreEqual("foobazfoofoobar", e8) - + /// This tests the three paths of String.concat w.r.t. array, list, seq + let execTest f expected arg = + let r1 = f (List.toSeq arg) + Assert.AreEqual(expected, r1) + + let r2 = f (List.toArray arg) + Assert.AreEqual(expected, r2) + + let r3 = f arg + Assert.AreEqual(expected, r3) + + do execTest (String.concat null) "foo" ["foo"] + do execTest (String.concat "") "" [] + do execTest (String.concat "foo") "" [] + do execTest (String.concat "") "" [null] + do execTest (String.concat "") "" [""] + do execTest (String.concat "foo") "bar" ["bar"] + do execTest (String.concat "foo") "bavfoobaz" ["bav"; "baz"] + do execTest (String.concat "foo") "foobazfoofoobar" [null;"baz";null;"bar"] + CheckThrowsArgumentNullException(fun () -> String.concat "foo" null |> ignore) [] From 16103cc38f251696cf96f6cda2477e1833277de1 Mon Sep 17 00:00:00 2001 From: Abel Braaksma Date: Fri, 19 Jun 2020 19:12:31 +0200 Subject: [PATCH 5/5] Remove "foo", "bax", "bar" in tests, making them more readable --- .../Microsoft.FSharp.Collections/StringModule.fs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 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 b71273d9c39..00ba1d5f8fc 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 @@ -34,16 +34,16 @@ type StringModule() = let r3 = f arg Assert.AreEqual(expected, r3) - do execTest (String.concat null) "foo" ["foo"] + do execTest (String.concat null) "world" ["world"] do execTest (String.concat "") "" [] - do execTest (String.concat "foo") "" [] + do execTest (String.concat "|||") "" [] do execTest (String.concat "") "" [null] do execTest (String.concat "") "" [""] - do execTest (String.concat "foo") "bar" ["bar"] - do execTest (String.concat "foo") "bavfoobaz" ["bav"; "baz"] - do execTest (String.concat "foo") "foobazfoofoobar" [null;"baz";null;"bar"] + do execTest (String.concat "|||") "apples" ["apples"] + do execTest (String.concat " ") "happy together" ["happy"; "together"] + do execTest (String.concat "Me! ") "Me! No, you. Me! Me! Oh, them." [null;"No, you. ";null;"Oh, them."] - CheckThrowsArgumentNullException(fun () -> String.concat "foo" null |> ignore) + CheckThrowsArgumentNullException(fun () -> String.concat "%%%" null |> ignore) [] member this.Iter() =