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

entry API v3 #921

Merged
merged 2 commits into from
Mar 19, 2015
Merged

entry API v3 #921

merged 2 commits into from
Mar 19, 2015

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Mar 1, 2015

Replace Entry::get with Entry::default and Entry::default_with for better ergonomics and clearer code.

*map.entry(key).default(0) += 1;
// vec![] doesn't even allocate, and is only 3 ptrs big.
map.entry(key).default(vec![]).push(val);
let val = map.entry(key).default_with(|| expensive(big, data));

Rendered

@Stebalien
Copy link
Contributor

How about or_insert/or_insert_with? Personally, I associate default() with Default::default() and, regardless, I don't expect a default() method to modify anything. Additionally, this allows default to be used to insert the Default::default() value:

map.entry(key).or_insert(0) += 1
let val = map.entry(key).or_insert_with(|| expensive(big, data));
map_of_vecs.entry(key).or_insert_default().push(0);

Drawback: or_insert... is long and ugly.

@reem
Copy link

reem commented Mar 1, 2015

It seems to me that we could get many of these benefits by replacing Result::unwrap_or_default with Result::default.

@huonw
Copy link
Member

huonw commented Mar 1, 2015

@reem that does something different (just returns the default value, ignoring the error, not inserting the default value into the data structure).

# Alternatives

Settle for Result chumpsville or abandon this sugar altogether. Truly, fates worse than death.

Copy link
Member

Choose a reason for hiding this comment

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

  • Pass more information into the default_with closure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's literally no information to pass for a vacantentry.

Copy link
Member

Choose a reason for hiding this comment

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

A reference to the key?

Copy link

Choose a reason for hiding this comment

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

@gankro Was this addressed somewhere off-thread or there's a technical reason it can't be done?

It would be neat to save a clone if the 'expensive' case requires the key to generate the value data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This API as a thin layer over Entry can't admit an &Key for the simple reason that Entry doesn't expose this. Otherwise it wasn't included in Entry simply because no one presented a compelling usecase (iirc every use of the old API I saw never used the key even though it was given). It's possible to add new methods that expose this functionality at a later date, though.

@reem
Copy link

reem commented Mar 2, 2015

@huonw You're right.

+1 to this RFC then, I think this is quite clean.

@Florob
Copy link

Florob commented Mar 2, 2015

I concur with @Stebalien, I find the default() name confusing.
Semantically this feels like unwrap_or_insert(x), or get_or_insert(x). Obviously that is unwieldy to write, but I personally prefer its explicitness.
Generally I'm +1 one though.

@andrew-d
Copy link

andrew-d commented Mar 2, 2015

Just as an interesting note, Python has something similar called setdefault.

}
}

#[unstable(feature = "collections",
Copy link
Member

Choose a reason for hiding this comment

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

you accidentally the tail end of this attr

@emberian
Copy link
Member

emberian commented Mar 2, 2015

This is nice, and hits all my uses of Entry.

@llogiq
Copy link
Contributor

llogiq commented Mar 2, 2015

In the Java world, there are some libraries aiming to enhance collections usage, which have wildly different APIs, e.g. trove has adjustOrPutValue, while koloboke and Java 8 have compute, computeIfPresent, computeIfAbsent and getOrDefault.

Python has a dict.get(key, defaultValue=None), which we cannot reproduce because without varargs, and which does not allow us to lazily compute the value (which, given that python is not in the 'optimiize the heck out of it' camp, is simply not a problem they choose to solve).

In this context, the suggested API feels right. It's certainly more orthogonal than any of the API designs of other languages.

👍

@dhardy
Copy link
Contributor

dhardy commented Mar 2, 2015

Really like the RFC.

Bike-shedding names: here's another suggestion (not necessarily better):

  • entry(key).with_default(vec![]).push(val);
  • let val = entry(key).with_default_fn(|| expensive(big, data));

@nikomatsakis
Copy link
Contributor

I like the idea but I am not wild about the name. Certainly before reading the RFC I had no idea what default should do, and did not expect it to insert into the table. or_insert seems pretty decent to me.

@arthurprs
Copy link

That would be a great addition, as others already voiced I'm unsure about the naming.

@aturon
Copy link
Member

aturon commented Mar 2, 2015

Well done, @gankro!

I agree with what many have said about the naming (especially overlap with the Default trait). What about defaulted? I could live with or_insert, as well.

@Gankra
Copy link
Contributor Author

Gankra commented Mar 2, 2015

👍 for or_insert as livable.

@cristicbz
Copy link

👍 can't get clearer than or_insert (and it's not that long either)

```
/// Ensures a value is in the entry by inserting the default if empty, and returns
/// a mutable reference to the value in the entry.
pub fn default(self. default: V) -> &'a mut V {
Copy link
Member

Choose a reason for hiding this comment

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

(&mut self, default: V) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

self for sure. Want to consume the entry.

@bluss
Copy link
Member

bluss commented Mar 3, 2015

Very nice API, I'd prefer the name .set_default(x). Prefer verbs. or_insert doesn't parse into anything sensible to me.. The reading "entry for key or insert x" is inevitable when you see .entry(key).or_insert(x) -- but we have already defined an entry to be either an occupied or a vacant entry, both valid.

@cristicbz
Copy link

or_insert() is a verb! and I think .entry(key).or_insert(...)
sounds like "get something or insert a value if it's not there".
set_default(x) doesn't make me think it returns the value was
previously there or a default. I could live with with_default as
well. But, yay bikeshedding!

On Tue, 3 Mar 2015 at 10:35 bluss notifications@github.com wrote:

Very nice API, I'd prefer the name .set_default(x). Prefer verbs.
or_insert doesn't parse into anything sensible to me..


Reply to this email directly or view it on GitHub
#921 (comment).

@aturon
Copy link
Member

aturon commented Mar 19, 2015

This RFC has been merged with overwhelming consensus. The merge updates the text to the or_insert and or_insert_with variants that many on this thread found appealing.

Tracking issue

@Centril Centril added the A-collections Proposals about collection APIs label Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Proposals about collection APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.