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

RFC: Add get! for Dicts (addresses #1529) #5519

Merged
merged 3 commits into from
Feb 9, 2014
Merged

RFC: Add get! for Dicts (addresses #1529) #5519

merged 3 commits into from
Feb 9, 2014

Conversation

kmsquire
Copy link
Member

This PR refactors setindex!(d::Dict, v, key) into setindex! and ht_keyindex2, where ht_keyindex2 returns either the current index of the key, or -index for the insertion location of a new key (after rehashing, etc).

(Compare with ht_keyindex, which returns -1 if the key is not in the hash table, and does not call rehash.)

This is used by get! to get the current or new location of a key.

The PR does not have any measurable impact on performance.

Related:

CC: @toivoh, @StefanKarpinski

@StefanKarpinski
Copy link
Sponsor Member

I'm for this. The case where the RHS is very expensive could be handled with a higher-order method:

get!(dict,key) do
  value
end

I'd also like to add syntax for this: dict[key] ?= value.

@toivoh
Copy link
Contributor

toivoh commented Jan 27, 2014

+1 for this and for syntax. I hope the latter would only evaluate the rhs
if needed.

@kmsquire
Copy link
Member Author

Okay, good.

After playing with this for DefaultDicts, I can also see the value in not evaluating the optional argument, as in Toivoh's @get or his request about the behavior of =?.

Questions:

  1. is this 0.3 material, or should it wait?
  2. should =? be included in this PR? (I think it could be separate?

#1764 could also be trivially reworked to use the new ht_keyindex2 code.

@toivoh
Copy link
Contributor

toivoh commented Jan 27, 2014

I think that ?= is a pretty major thing that definitely deserves its own
PR. (Do you actually know how to do it?)

@kmsquire
Copy link
Member Author

I think that ?= is a pretty major thing that definitely deserves its own PR. (Do you actually know how to do it?)

Agree. I don't know how to do it off-hand, but I think I could probably figure it out. But if you (or someone else) want to take a shot, feel free! It might be a couple of simple additions to julia-syntax.scm

@kmsquire
Copy link
Member Author

I've added the do-block syntax of get! that @StefanKarpinski suggested, as well as tests and docs.

* Refactored setindex! into ht_keyindex2, for use by get! and setindex!
Also added get, get! for WeakKeyDicts
@kmsquire
Copy link
Member Author

kmsquire commented Feb 8, 2014

In response to #1764 (comment), I added do-block syntax for get as well.

Also added get! and a do-block version of get for WeakKeyDicts. Haven't had time for ObjectIdDicts yet, though I can add that if there's interest.

@StefanKarpinski
Copy link
Sponsor Member

I like it. I'm down do merge this. @JeffBezanson?

@JeffBezanson
Copy link
Sponsor Member

Ok by me.

StefanKarpinski added a commit that referenced this pull request Feb 9, 2014
RFC: Add `get!` for Dicts (addresses #1529)
@StefanKarpinski StefanKarpinski merged commit a9f702b into master Feb 9, 2014
@kmsquire kmsquire deleted the kms/get! branch August 10, 2014 20:04
@kmsquire kmsquire mentioned this pull request Feb 4, 2015
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.

None yet

5 participants