Uh oh!
There was an error while loading. Please reload this page.
Cleanup collection functions - #1347
Conversation
forki
commented
Jul 13, 2016
- Optimizing namespaces
- inlining LanguagePrimitives.ErrorStrings.InputMustBeNonNegativeString
- using Array.empty
dsyme
commented
Jul 17, 2016
Fakey AppVeyor results upoading: |
forki
commented
Jul 18, 2016
That's unrelated, right? |
dsyme
commented
Jul 18, 2016
@forki Yes, unrelated. It's been seen before, don't understand why it happens. |
There was a problem hiding this comment.
I think we don't want to do this for non-inlined functions. There is an outstanding bug where those "inlined" strings are non-localized (e.g. in Italian). So where possible we use the SR.GetString call
There was a problem hiding this comment.
really? is this bug tracked here?
There was a problem hiding this comment.
thanks. let's try to fix that one then ;-)
There was a problem hiding this comment.
Actually IIRC it's hard to fix - amking those "inlined" values into functions involves adding surface area to FSharp.Core. There was some reason we pulled out of fixing it though I'm not sure why.
Is there any particular reason you want to use the direct string?
There was a problem hiding this comment.
I guess I can revert that.
On Jul 22, 2016 11:39 AM, "Don Syme" notifications@github.com wrote:
In src/fsharp/FSharp.Core/array.fs
#1347 (comment)
:@@ -120,8 +121,8 @@ namespace Microsoft.FSharp.Collections
[<CompiledName("Replicate")>] let replicate count x =
if count < 0 then invalidArg "count" (SR.GetString(SR.inputMustBeNonNegative))let arr = (Microsoft.FSharp.Primitives.Basics.Array.zeroCreateUnchecked count : 'T array)if count < 0 then invalidArg "count" LanguagePrimitives.ErrorStrings.InputMustBeNonNegativeStringActually IIRC it's hard to fix - amking those "inlined" values into
functions involves adding surface area to FSharp.Core. There was some
reason we pulled out of fixing it though I'm not sure why.Is there any particular reason you want to use the direct string?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/Microsoft/visualfsharp/pull/1347/files/057f9aa43438c2f76a1505ddb288de1887437824#r71852825,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNEMFoQrpWeBPeW2UZ7Ti1HLP2Z3Tks5qYI_ggaJpZM4JLdjM
.
There was a problem hiding this comment.
ok reverted that part. ready for review.
KevinRansom
commented
Aug 1, 2016
@dotnet-bot test this please |
KevinRansom
commented
Aug 1, 2016
@forki -- When I fixed the merge errors on this one I get this build error: I like all of the cleanup in this PR, it all looks good except for this one thing. |
forki
commented
Aug 2, 2016
Let me rebase this. |
forki
commented
Aug 2, 2016
ok should work now |
dsyme
commented
Aug 3, 2016
@forki Thanks for this! |