Skip to content
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

Make Map insertion functions total #3203

Merged

Conversation

rkallos
Copy link
Contributor

@rkallos rkallos commented Jun 26, 2019

Thanks to #3201, it's now possible to make Map.insert & friends total functions. Not only that, but Map.insert should be slightly faster in the case where the insertion causes the underlying array to grow, as it is no longer necessary to do the lookup for the final value.

I suspect this would constitute a pretty large breaking change, as I'm sure code like try map.insert(k, v)? end is pretty common, and after this change it would be a compile-time error.

This PR is incomplete. There are some more partial-not-partial functions that can be fixed, like Map.insert_if_absent.

@rkallos rkallos force-pushed the feature/no-more-partial-map-insert branch from f2bd17b to a87a371 Compare June 26, 2019 15:05
@SeanTAllen SeanTAllen added the do not merge This PR should not be merged at this time label Jun 26, 2019
@SeanTAllen
Copy link
Member

@rkallos you'll need to rebase against master to pick up the removal of pcre2 so that when you start up on this again, it can work with the latest CI images.

@rkallos rkallos force-pushed the feature/no-more-partial-map-insert branch 3 times, most recently from d562073 to 098dec1 Compare July 19, 2019 15:15
@rkallos rkallos changed the title DO NOT MERGE: Make Map.insert total Make Map insertion functions total Jul 19, 2019
@rkallos
Copy link
Contributor Author

rkallos commented Jul 19, 2019

Thanks @SeanTAllen for adding the label and letting me know to rebase. I think this is ready for review now.

@SeanTAllen SeanTAllen added needs discussion during sync and removed do not merge This PR should not be merged at this time labels Jul 23, 2019
@SeanTAllen SeanTAllen requested a review from jemc July 23, 2019 00:48
packages/collections/map.pony Outdated Show resolved Hide resolved
packages/collections/map.pony Outdated Show resolved Hide resolved
packages/collections/map.pony Outdated Show resolved Hide resolved
packages/collections/map.pony Outdated Show resolved Hide resolved
Copy link
Member

@jemc jemc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a beautiful thing!

@rkallos rkallos force-pushed the feature/no-more-partial-map-insert branch from 811b3b8 to acea091 Compare July 23, 2019 18:16
@rkallos
Copy link
Contributor Author

rkallos commented Jul 23, 2019

Rebased and squashed :)

@SeanTAllen
Copy link
Member

@rkallos can you add release notes for this change? including "what your code looks like before and after" so folks know how to update?

@rkallos
Copy link
Contributor Author

rkallos commented Jul 23, 2019

Before this change, code that inserts into maps might look like:

try map.insert("key", 1)? end
try map.insert_if_absent("key", 5)? end
try
  map.upsert("key", 1, {(current, provided) => current + provided})?
end

After this change, the question marks and the surrounding try expressions can be removed. Respectively, the above blocks become:

map.insert("key", 1)
map.insert_if_absent("key", 5)
map.upsert("key", 1, {(current, provided) => current + provided})?

@SeanTAllen SeanTAllen merged commit fd5acec into ponylang:master Jul 23, 2019
SeanTAllen added a commit that referenced this pull request Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants