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

Shorten type param names #261

Merged
merged 2 commits into from Mar 16, 2017

Conversation

Projects
None yet
3 participants
@leshow
Copy link
Contributor

leshow commented Mar 15, 2017

I picked the most obvious letters to me. Mapping operations; F for function, for anything that returns a bool; P for predicate, etc. I made sure to change the associated lifetimes to match up and fix any formatting changes. Let me know if there's anything you'd like to change.

#249

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Mar 15, 2017

I'm not fond of changing ITEM to I -- that evokes "iterator" to my mind, especially when we're dealing with a library of iterators here. I'd prefer plain T for items, as the idiomatic type placeholder in Rust.

@@ -24,9 +24,9 @@ pub fn new<M, FILTER_OP>(base: M, filter_op: FILTER_OP) -> FilterMap<M, FILTER_O
}
}

impl<M, FILTER_OP, R> ParallelIterator for FilterMap<M, FILTER_OP>
impl<M, P, R> ParallelIterator for FilterMap<M, P>

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 15, 2017

Member

I no longer recall why I used M for the "base" operations, but it doesn't seem terribly intuitive to me. I see that the standard library uses I, perhaps we should use that (after adopting @cuviper's suggestion to change ITEM to T)?

impl<'f, ITEM, MAPPED_ITEM, C, FILTER_OP> Consumer<ITEM> for FilterMapConsumer<'f, C, FILTER_OP>
where C: Consumer<MAPPED_ITEM>,
FILTER_OP: Fn(ITEM) -> Option<MAPPED_ITEM> + Sync + 'f
impl<'p, I, MI, C, P> Consumer<I> for FilterMapConsumer<'p, C, P>

This comment has been minimized.

@nikomatsakis

nikomatsakis Mar 15, 2017

Member

If we were using T for Item, I think I'd prefer U for MAPPED_ITEM. If we're going into single letter territory, might as well go there, I guess. Somewhat oddly (to my mind), the standard library seems to adopt B for this purpose, though.

@nikomatsakis

This comment has been minimized.

Copy link
Member

nikomatsakis commented Mar 15, 2017

OK, looking over the changes, I'm torn. I do think the existing names like like MAP_OP and REDUCE_OP are not great. But the single-letter names sure do feel cryptic, particularly when you have a whole bunch in sequence (I'd be inclined to publish a "guide to the letters" in the docs somewhere, but it feels like if you need a guide to your names, you could have picked better names). Currently though we seem to have a mix, which is also not a win. I think all things considered I'm inclined to adopt this branch (after we finish bike-shedding the particular choices), since I'm on the fence and it aligns better with the prevailing style, but I'm mildly tempted to try and find better "one word" names (e.g., FUNC instead of MAP_OP) as an alternative.

@leshow

This comment has been minimized.

Copy link
Contributor Author

leshow commented Mar 15, 2017

I think those are good suggestions, (ITEM -> T, MAPPED_ITEM -> U) I can move on those later today so we can see what it looks like.

There is a few other cases where I ended up with a 2 letter params.
IDENTITY -> ID
OP -> OP
C_ITEM -> CI
PAR_ITER -> PI (following the prev suggestion this could just be I)

For C_ITEM, it occupies the same position as MAPPED_ITEM, so I think that should also become U if we make the aforementioned change. I don't see the 2 letter params as a problem, but if you want to strictly pursue the 1 character generic name then those will have to find replacements.

About documentation on the param names, I'm not an expert Rust developer and this library is new to me so my opinion may be worthwhile here, having the name MAPPED_ITEM didn't help me, looking at the constraints in the where clause did though.

@leshow leshow force-pushed the leshow:master branch 2 times, most recently from be7f731 to 85eb7a8 Mar 15, 2017

MAPPED_ITEM -> U, ITEM -> T, all iterators -> I
match some 'm lifetimes -> 'f to F

@leshow leshow force-pushed the leshow:master branch from 85eb7a8 to 6d0994a Mar 16, 2017

@nikomatsakis

This comment has been minimized.

Copy link
Member

nikomatsakis commented Mar 16, 2017

Looks pretty good to me. I'm inclined to merge.

@nikomatsakis

This comment has been minimized.

Copy link
Member

nikomatsakis commented Mar 16, 2017

@cuviper you want to give one more look, or are you satisfied?

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Mar 16, 2017

I'm happy with this.

@nikomatsakis nikomatsakis merged commit cb1f3a8 into rayon-rs:master Mar 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

cuviper added a commit to cuviper/rayon that referenced this pull request Mar 16, 2017

@leshow

This comment has been minimized.

Copy link
Contributor Author

leshow commented Mar 16, 2017

Thank you!

cuviper added a commit to cuviper/rayon that referenced this pull request Mar 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.