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

map_entry warning does not consider possible intervening code #5696

Closed
jmafc opened this issue Jun 8, 2020 · 3 comments
Closed

map_entry warning does not consider possible intervening code #5696

jmafc opened this issue Jun 8, 2020 · 3 comments

Comments

@jmafc
Copy link

jmafc commented Jun 8, 2020

Version: clippy 0.0.212 (d4092ac 2020-05-11)
I get the following warning:

172 | /         if !seen.contains_key(&url) {
173 | |             v.push(Href {url: url.clone(), title: row.get("title")});
174 | |             seen.insert(url, 1);
175 | |         }
    | |_________^ consider using `seen.entry(url)`

The warning doesn't take into account that the contains_key() / insert() sequence has some code that is necessary in between (it could also apply if the push() was placed after the insert(). For context, this is inside a loop iterating over rows and the Hashmap seen is used to prevent inserting duplicates into the vector v. AFAICT (I've tried several variations of using Entry methods), using the suggested or_insert() does not provide a solution.

@ghost
Copy link

ghost commented Jun 9, 2020

What about...

if let Entry::Vacant(entry) = seen.entry(url) {
    v.push(Href {url: url.clone(), title: row.get("title")});
    entry.insert(1);
}

Now you're only looking up the key once so it should be faster. Aside from looking nicer.

@jmafc
Copy link
Author

jmafc commented Jun 9, 2020

Yes, that works (except for having to use url.clone() in the first rather than the second one). I'm still wondering, though, whether the warning is always valid, irrespective of other code in the block following the contains_key().

@ghost
Copy link

ghost commented Jun 12, 2020

There are cases. entry() mutably borrows the Hashmap so the suggestion is invalid if you're using the hashmap for something else in the block. See #4674.

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

No branches or pull requests

1 participant