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

Redo the interface of CFDictionary and CFMutableDictionary #189

Merged
merged 2 commits into from Feb 7, 2018

Conversation

@jrmuizel
Copy link
Collaborator

jrmuizel commented Feb 3, 2018

This is a pretty big change to take advantge of the type parameters.
It simplifies things and makes them more safe.


This change is Reviewable

@jrmuizel
Copy link
Collaborator Author

jrmuizel commented Feb 3, 2018

The shared types (FromVoid, ItemRef) from array should probably be moved into base

@jrmuizel
Copy link
Collaborator Author

jrmuizel commented Feb 3, 2018

The ergonomics of using this new interface with kSecImportItemLabel constants style constants as keys isn't great. I'll try to think of what can be done.

Copy link
Member

jdm left a comment

This is a nice improvement.


/// A trait describing how to convert from the stored *const c_void to the desired T
pub trait ToVoid {
fn to_void<'a>(&self) -> *const c_void;

This comment has been minimized.

@jdm

jdm Feb 6, 2018

Member

I don't understand the purpose of the lifetime.

}

#[inline]
pub fn find(&self, key: *const c_void) -> Option<*const c_void> {
pub fn find<'a>(&self, key: &K) -> Option<ItemRef<'a, V>> where V: FromVoid, K: ToVoid {

This comment has been minimized.

@jdm

jdm Feb 6, 2018

Member

Seeing no use of 'a in the input arguments is surprising. Should it be related to &self?


#[inline]
pub fn find(&self, key: *const c_void) -> Option<*const c_void> {
pub fn find<'a>(&self, key: &K) -> Option<ItemRef<'a, V>> where V: FromVoid, K: ToVoid {

This comment has been minimized.

@jdm

jdm Feb 6, 2018

Member

Same question about the unused 'a.

@jrmuizel jrmuizel force-pushed the jrmuizel:more-dict branch 2 times, most recently from d4d7885 to ac44497 Feb 7, 2018
jrmuizel added 2 commits Feb 7, 2018
This will let us reuse them in dictionary.rs
This is a pretty big change to take advantge of the type parameters.
It simplifies things and makes them more safe.
@jrmuizel jrmuizel force-pushed the jrmuizel:more-dict branch from ac44497 to 97ec75c Feb 7, 2018
@jrmuizel
Copy link
Collaborator Author

jrmuizel commented Feb 7, 2018

This is ready for review now.

@jrmuizel jrmuizel changed the title [RFC] Redo the interface of CFDictionary and CFMutableDictionary Redo the interface of CFDictionary and CFMutableDictionary Feb 7, 2018
@jdm
Copy link
Member

jdm commented Feb 7, 2018

@bors-servo r+
Much better.

@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2018

📌 Commit 97ec75c has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2018

Testing commit 97ec75c with merge 070a866...

bors-servo added a commit that referenced this pull request Feb 7, 2018
Redo the interface of CFDictionary and CFMutableDictionary

This is a pretty big change to take advantge of the type parameters.
It simplifies things and makes them more safe.

<!-- 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/189)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2018

☀️ Test successful - status-travis
Approved by: jdm
Pushing 070a866 to master...

@bors-servo bors-servo merged commit 97ec75c into servo:master Feb 7, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.