-
Notifications
You must be signed in to change notification settings - Fork 772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't create dict for empty lists #5348
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better investigate the failures.
Those failures don't really look related. Let's try again. @dotnet build test this |
@dotnet-bot test this please |
@@ -405,10 +405,12 @@ namespace Microsoft.FSharp.Collections | |||
loop 0 | |||
|
|||
let inline groupByImpl (comparer:IEqualityComparer<'SafeKey>) (keyf:'T->'SafeKey) (getKey:'SafeKey->'Key) (array: 'T[]) = | |||
let length = array.Length | |||
if length = 0 then Microsoft.FSharp.Primitives.Basics.Array.zeroCreateUnchecked 0 else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's time to add empty arrays singleton? #5066 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. I could use them in many places
this is ready |
@forki thanks for this mate. |
@KevinRansom As mentioned in #5942 this is an incorrect optimization. We should revert and publish a new version of FSharp.Core assuming this has made it out into the wild (I think it has) |
ouch. apologies. @cartermp we also need some kind of test here |
* Fix and tests, unbuilt and untested * Correct one of the tests Got the order wrong for the output of `countBy`. * Repair the groupBy test groupBy test was bad: it assumed (ridiculously) that groupBy wouldn't iterate the original sequence even though it somehow knew the groups in the sequence.
… (dotnet#5947) * Fix and tests, unbuilt and untested * Correct one of the tests Got the order wrong for the output of `countBy`. * Repair the groupBy test groupBy test was bad: it assumed (ridiculously) that groupBy wouldn't iterate the original sequence even though it somehow knew the groups in the sequence.
As in most other list operation implementations we should shortcut empty lists