Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upHigher level dictionary find/contains_key #129
Conversation
| let value = self.find(key); | ||
| if value.is_none() { | ||
| panic!("No entry found for key {:p}", key); | ||
| match self.find(key) { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
faern
Nov 22, 2017
Author
Contributor
Indeed. I just kept the panic! existing before this PR. With expect I can't do the formatting as easily (will need an extra format!) and I did not want to remove the printing of the pointer. I don't see the point in printing the pointer but maybe there is one so I kept it. If not, the entire implementation can simply be
self.find(key).expect("No entry found for key")To be honest I don't see the reason to have this get at all. Much more explicit to use find and have to unwrap yourself if you want panicking behavior.
I guess it would break some stuff, but I think it would be cleaner to get rid of this panicking version altogether and rename find to get. From my experience get is a more common name with dictionaries, plus it would match better with the underlying CFDictionaryGetValueIfPresent.
This comment has been minimized.
This comment has been minimized.
KiChjang
Nov 22, 2017
Member
Ah, so there's the format! macro that you can use if formatting is the problem you're facing.
This comment has been minimized.
This comment has been minimized.
faern
Nov 22, 2017
Author
Contributor
Yes. So under the conditions that get should remain and that it should panic with the pointer in the message, now it probably has the simplest implementation.
This comment has been minimized.
This comment has been minimized.
faern
Nov 22, 2017
Author
Contributor
However, this version is forced to always allocate and format the string, even in the happy case. So the previous solution probably had better performance.
|
@bors-servo: r+ |
|
|
Higher level dictionary find/contains_key I'm not very familiar with Core Foundation yet, so tell me if this is not a very helpful addition. Adding versions of find and contains_key that can take any `TCFType` and automatically converts it to the raw pointer of its corresponding ref type. It looks like dictionaries will mostly contain types implementing this trait, and thus these versions of the methods can make it a bit more ergonomic to use the dictionary. The best would of course be if we could return the correct type as well, but I guess that is not possible with the available information. Better suggestions for the names are welcome :) <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/core-foundation-rs/129) <!-- Reviewable:end -->
|
|
faern commentedNov 22, 2017
•
edited by larsbergstrom
I'm not very familiar with Core Foundation yet, so tell me if this is not a very helpful addition.
Adding versions of find and contains_key that can take any
TCFTypeand automatically converts it to the raw pointer of its corresponding ref type. It looks like dictionaries will mostly contain types implementing this trait, and thus these versions of the methods can make it a bit more ergonomic to use the dictionary. The best would of course be if we could return the correct type as well, but I guess that is not possible with the available information.Better suggestions for the names are welcome :)
This change is