daml
daml copied to clipboard
digital-asset
Optimise `fromListWith` in `Map` and `TextMap` for list-like structures.
Optimise fromListWith to prepend the singleton list to the accumulated list instead of appending to it.
It also does not make sense that fromListWith appends the new values and insertWith prepends.
Hi @yangzai, thanks for your PR. Before merging it we need to discuss internally about the implications for existing uses of these functions, but for now I'll kick off the CI tests
![azure-pipelines[bot] avatar](https://avatars.githubusercontent.com/in/9426?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hi @yangzai, thanks for your PR. Before merging it we need to discuss internally about the implications for existing uses of these functions, but for now I'll kick off the CI tests
We could suggest users to flip their combining function during migration if they want to keep to the old behaviour.
@yangzai Sorry for the lack of update here. We can't really make a semantic change while keeping the function name the same, as this would change existing code's behaviour.
We're currently looking into providing fromListWithL and fromListWithR as a future release, but this is still being discussed.
@yangzai Sorry for the lack of update here. We can't really make a semantic change while keeping the function name the same, as this would change existing code's behaviour.
We're currently looking into providing fromListWithL and fromListWithR as a future release, but this is still being discussed.
@basvangijzel-DA noted. I can work on the necessary refactoring while the decision is being made.
@yangzai Okay, great if you're happy to do the refactoring. The following would we be good to have:
- define fromListWithL and fromListWithR as new functions (you can copy paste fromWithList as a base)
- change the definition of fromListWith to point to fromListWithL (please doublecheck that I picked the right one)
Just FYI: In a later major release (not for this PR) we would delete fromListWith, but provide a rewrite rule in the compiler/IDE to help with a fix.
@yangzai Okay, great if you're happy to do the refactoring. The following would we be good to have:
- define fromListWithL and fromListWithR as new functions (you can copy paste fromWithList as a base)
- change the definition of fromListWith to point to fromListWithL (please doublecheck that I picked the right one)
Just FYI: In a later major release (not for this PR) we would delete fromListWith, but provide a rewrite rule in the compiler/IDE to help with a fix.
@basvangijzel-DA I have pointed fromListWith to fromListWithR instead. As a sanity check testfromListWithR should match with the old testfromListWith.
@yangzai, there's a failing test, //compiler/damlc/tests:platform-independence-dar-hash-file-matches. The reason is that your change affects the package id of daml-stdlib, which means that the golden file needs to be updated. Since I can't push to your branch and I'm not sure you'll be able to run the bazel rule to update the golden file, I've cherry-picked your commits and updated the golden file in PR https://github.com/digital-asset/daml/pull/18753
