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

Make Iterator naming more consistent #11001

Closed
wants to merge 1 commit into from

Conversation

DaGenix
Copy link

@DaGenix DaGenix commented Dec 16, 2013

Most Iterators renamed to make their naming more consistent. Most significantly, the Iterator and Iter suffixes have been completely removed.

@alexcrichton
Copy link
Member

I think this should be discussed as to whether this is the right direction to go in.

@DaGenix
Copy link
Author

DaGenix commented Dec 16, 2013

I don't have an opinion as to what naming convention is better. Is this the appropriate place for that discussion to occur or is ML better?

@DaGenix
Copy link
Author

DaGenix commented Dec 21, 2013

I rebased to origin/master and also renamed all Iterators without a suffix (that I could find) to have a suffix of Iterator. I didn't do extensive testing since I mostly mean for this to be an RFC at this point.

@DaGenix
Copy link
Author

DaGenix commented Jan 4, 2014

So, I said I was giving up on this on the mailing list, but I decided to give it one more go. In my attempt to get rid of an eye-sore that I feel responsible for adding, I've ended up with this PR that renames almost every existing iterator. Anyway, I removed the "Iterator" suffix from every iterator, but left the traits in iter.rs untouched. I tried to rename all of the iterators to be more descriptive over what they are iterating. So, I renamed TreeMapIterator to Pairs since that is what that iterator is actually producing: key-value pairs. For iterators that are simply iterating over all the elements of a container, I renamed the iterator to "Elements". I also tried to make everything more consistent - eg there were 3 different naming conventions for reverse iterators over X: RevX, XRev, and XR. I just put "Rev" as the first part of the name.

Anyway, here is a chart with all the renaming that I did. I think the chart is almost in sync with the pull request, but I don't guarantee they are 100% the same. The chart is better for critiquing (or even better - suggesting improved names) than for reviewing the pull request:

File original name new name
src/libextra/bitv.rs BitvIterator Elements
src/libextra/bitv.rs BitvSetIterator SetElements
src/libextra/dlist.rs DListIterator Elements
src/libextra/dlist.rs MutDListIterator MutElements
src/libextra/dlist.rs MoveIterator MoveElements
src/libextra/enum_set.rs EnumSetIterator Elements
src/libextra/glob.rs GlobIterator Paths
src/libextra/priority_queue.rs PriorityQueueIterator Elements
src/libextra/ringbuf.rs RingBufIterator Elements
src/libextra/ringbuf.rs RingBufMutIterator MutElements
src/libextra/smallintmap.rs SmallIntMapIterator Pairs
src/libextra/smallintmap.rs SmallIntMapMutIterator MutPairs
src/libextra/smallintmap.rs SmallIntMapRevIterator RevPairs
src/libextra/smallintmap.rs SmallIntMapMutRevIterator RevMutPairs
src/libextra/treemap.rs TreeMapIterator Pairs
src/libextra/treemap.rs TreeMapRevIterator RevPairs
src/libextra/treemap.rs TreeMapMoveIterator MovePairs
src/libextra/treemap.rs TreeMapMutRevIterator RevMutPairs
src/libextra/treemap.rs TreeSetIterator SetElements
src/libextra/treemap.rs TreeSetRevIterator RevSetElements
src/libextra/treemap.rs Difference DifferenceElements
src/libextra/treemap.rs SymDifference SymDifferenceElements
src/libextra/treemap.rs Intersection IntersectionElements
src/libextra/treemap.rs Union UnionElements
src/librustc/middle/trans/basic_block.rs PredIterator Preds
src/librustc/middle/trans/value.rs UserIterator Users
src/libstd/c_str.rs CStringIterator CChars
src/libstd/comm/mod.rs PortIterator Messages
src/libstd/comm/select.rs PacketIterator Packets
src/libstd/hashmap.rs HashMapIterator Pairs
src/libstd/hashmap.rs HashMapMutIterator MutPairs
src/libstd/hashmap.rs HashMapMoveIterator MovePairs
src/libstd/hashmap.rs HashSetIterator SetElements
src/libstd/hashmap.rs HashSetMoveIterator SetMoveElements
src/libstd/hashmap.rs SetAlgebraIter SetAlgebraElements
src/libstd/io/extensions.rs ByteIterator Bytes
src/libstd/io/fs.rs WalkIterator Directories
src/libstd/io/mod.rs LineIterator Lines
src/libstd/io/mod.rs IncomingIterator IncomingConnections
src/libstd/option.rs OptionIterator Element
src/libstd/path/posix.rs ComponentIter Components
src/libstd/path/posix.rs RevComponentIter RevComponents
src/libstd/path/posix.rs StrComponentIter StrComponents
src/libstd/path/posix.rs RevStrComponentIter RevStrComponents
src/libstd/path/windows.rs StrComponentIter StrComponents
src/libstd/path/windows.rs RevStrComponentIter RevStrComponents
src/libstd/path/windows.rs ComponentIter Components
src/libstd/path/windows.rs RevComponentIter RevComponents
src/libstd/rt/task.rs BlockedTaskIterator BlockedTasks
src/libstd/str.rs CharIterator Chars
src/libstd/str.rs CharOffsetIterator CharOffsets
src/libstd/str.rs CharSplitIterator CharSplits
src/libstd/str.rs CharSplitNIterator CharSplitsN
src/libstd/str.rs MatchesIndexIterator MatchIndices
src/libstd/str.rs StrSplitIterator StrSplits
src/libstd/str.rs NormalizationIterator Normalizations
src/libstd/str.rs CharRevIterator RevChars
src/libstd/str.rs CharOffsetRevIterator RevCharOffsets
src/libstd/str.rs ByteIterator Bytes
src/libstd/str.rs ByteRevIterator RevBytes
src/libstd/str.rs CharRSplitIterator RevCharSplits
src/libstd/str.rs WordIterator Words
src/libstd/str.rs AnyLineIterator AnyLines
src/libstd/trie.rs TrieMapIterator Pairs
src/libstd/trie.rs TrieMapIterator MutPairs
src/libstd/trie.rs TrieSetIterator SetElements
src/libstd/vec.rs SplitIterator Splits
src/libstd/vec.rs RSplitIterator RevSplits
src/libstd/vec.rs WindowIter Windows
src/libstd/vec.rs ChunkIter Chunks
src/libstd/vec.rs VecIterator Elements
src/libstd/vec.rs VecMutIterator MutElements
src/libstd/vec.rs MutSplitIterator MutSplits
src/libstd/vec.rs MutChunkIter MutChunks
src/libstd/vec.rs MoveIterator MoveElements
src/libstd/vec.rs RevIterator RevElements
src/libstd/vec.rs MutRevIterator RevMutElements
src/libstd/vec.rs MoveRevIterator RevMoveElements
src/libsyntax/opt_vec.rs OptVecIterator Elements
src/libsyntax/util/small_vector.rs SmallVectorMoveIterator MoveElements

In closing, I hope I never see an iterator again.

@emberian
Copy link
Member

emberian commented Jan 4, 2014

Ohh I like this quite a bit. Actually puts the module system to use! I was indifferent to the earlier renamings, thinking all they did was shuffle things about needlessly, but these names feel right to me.

@liigo
Copy link
Contributor

liigo commented Jan 4, 2014

Items is shorter than Elements

@DaGenix
Copy link
Author

DaGenix commented Jan 4, 2014

If the consensus is that 'Items' is better than 'Elements', I'm happy to change it up. Personally, I prefer 'Elements' a bit more than 'Items', but I don't have a strong opinion either way. Due to type inference, the vast majority of these types are never actually referenced outside of the module that they are declared in so, having a slightly longer name isn't much of a burden for the average user.

@huonw
Copy link
Member

huonw commented Jan 4, 2014

/me joins the bikeshed

I'm very happy with Elements: one normally says "the elements of a vector" not "the items of a vector" (at least, that's what I say) and one doesn't write the iterator types out much anyway.

@spaolacci
Copy link
Contributor

Abbreviating Elements to Elems is another possibility. It would go one step further into the "abbreviation everywhere" direction, though (e.g RevMutElems).

@liigo
Copy link
Contributor

liigo commented Jan 5, 2014

No, it's not "the Elements of a vector", it is "the Items from a iterater".
We are talking about iterater not vector, isn't it?
在 2014年1月4日 下午2:56,"Huon Wilson" notifications@github.com写道:

/me joins the bikeshed

I'm very happy with Elements: one normally says "the elements of a vector"
not "the items of a vector" (at least, that's what I say) and one doesn't
write the iterator types out much anyway.


Reply to this email directly or view it on GitHubhttps://github.com//pull/11001#issuecomment-31573109
.

@huonw
Copy link
Member

huonw commented Jan 5, 2014

These are iterators over the elements of various data-structures. (If we called it "items" as in "items of an iterator" then we could call every iterator "items", because all iterators have items.)

@DaGenix
Copy link
Author

DaGenix commented Jan 7, 2014

Rebased after #11321 landed to fix merge conflicts and rename the new iterator. I also updated my comment above with all of the details on what iterators were renamed to.

@huonw
Copy link
Member

huonw commented Jan 7, 2014

There's also #11342 which does to std::trie what #11321 did to extra::treemap (sorry about the rebase work!).

@DaGenix
Copy link
Author

DaGenix commented Jan 7, 2014

Ok, cool, np. I'll rebase again after that lands.

@alexcrichton
Copy link
Member

I am adding this to the next meeting agenda

@alexcrichton
Copy link
Member

(thanks for the renaming overview, it's very convenient to have!)

@DaGenix
Copy link
Author

DaGenix commented Jan 8, 2014

Rebased and updated my comment above for the new iterator, TrieMapMutIterator -> MutPairs.

Thanks for adding it to the agenda!

@metajack
Copy link
Contributor

@DaGenix would you be willing to do the same treatment to Servo as well? :)

@alexcrichton
Copy link
Member

We discussed this at today's meeting, and we're in favor of the general removal of the name iterator. We felt that Items was slightly better than Elements because it is indeed shorter and elements/items are essentially the same thing.

One thing I'm also interested in establishing a set of naming conventions for iterators. From your renamings, I get this impression:

  1. If the iterator is yielding something that can be described with a noun, the iterator should be called the pluralization of the noun (an iterator yielding words is called Words)
  2. An iterator over the members of a container should have the base name of Items, with different flavors deriving from this name. Different flavors are applied in order of top-to-bottom in this list
    • Moving iterators have the prefix of Move
    • If the default iterator yields an immutable reference, an iterator yielding a mutable reference has the prefix Mut
    • Reverse iterators have the prefix of Rev

Do those rules sound reasonable to you? I'll add them to the style guide once this lands.

@DaGenix
Copy link
Author

DaGenix commented Jan 15, 2014

@alexcrichton Sounds good to me! I'll work on rebasing this to change Elements to Items.

That is what I was thinking regarding rules when I did the renaming. My thinking with "SetElements" (from treemap / hashmap) is that just "Elements" would be confusing because those modules define two separate types - a Map type and a Set type - and it would not be all that clear which of those an iterator "Elements" belonged to. So, I broke the general rules you outlined above because I think "SetElements" is easier to read for that reason. So, if that makes sense, maybe their is a final rule:

  • If a module defines multiple types and there is a possibility of confusion using these rules, the iterators for the non-primary types should have a prefix to make it clear which type they are related to.
  • or, maybe something more general: If these rules would result in a name that might cause confusion, pick a less confusing name.

@metajack I'll will take a look a Servo. I've never built it before, so, we'll see how that goes.

@alexcrichton
Copy link
Member

Sounds good to me, and thanks so much for doing this work! I really like this PR :)

@DaGenix
Copy link
Author

DaGenix commented Jan 15, 2014

Thanks, np. This ended up being quite a bit bigger than I originally intended, but I'm quite happy with the result.

@DaGenix
Copy link
Author

DaGenix commented Jan 15, 2014

Rebased to change Elements -> Items.

@tikue
Copy link
Contributor

tikue commented Jan 15, 2014

I feel fairly strongly that Pairs is the wrong choice for key-value pairs. While they are, indeed, a pair, I think they have a specific relationship that Pair de-emphasizes. I think something like Entries or Records would be more suitable.

@sfackler
Copy link
Member

I agree with @tikue here. Java uses Entry as the name of the type and entries as the name of the method.

@alexcrichton
Copy link
Member

Sounds reasonable to me, @DaGenix, would you mind renaming pairs to entries?

@olivren
Copy link
Contributor

olivren commented Jan 15, 2014

If the iterator is named after the iterated elements, I'd expect bitv's iterator to be named bits.

@DaGenix
Copy link
Author

DaGenix commented Jan 16, 2014

Sounds good, I will rebase with changes.

It makes sense to me that BitvIterator becomes Bits. But what does BitvSetIterator become?

@olivren
Copy link
Contributor

olivren commented Jan 16, 2014

Given that BitvSet is a container for uints, the name of the iterator could be UInts.

@alexcrichton
Copy link
Member

According to our rules above, the BitvSetIterator sounds like it should become SetBits which doesn't sound too bad to me?

@olivren
Copy link
Contributor

olivren commented Jan 16, 2014

That would break the first rule. BitvSet yields bit indices (as uints
values), not bits. Other valid names could be BitIndices, BitPositions or
BitOffsets.

@DaGenix
Copy link
Author

DaGenix commented Jan 18, 2014

Rebased again to rename Pairs -> Entries, bitv::Items -> bitv::Bits, and bitv::SetItems -> bitv::BitPositions. Thanks for everyone's input!

@huonw
Copy link
Member

huonw commented Jan 18, 2014

This needs another rebase, unfortunately.

(Probably my most recent batch of trie iterator changes... sorry :( )

Rename existing iterators to get rid of the Iterator suffix and to
give them names that better describe the things being iterated over.
@DaGenix
Copy link
Author

DaGenix commented Jan 18, 2014

Ok, cool, np. Rebased.

bors added a commit that referenced this pull request Jan 18, 2014
Most Iterators renamed to make their naming more consistent. Most significantly, the Iterator and Iter suffixes have been completely removed.
@bors bors closed this Jan 18, 2014
@alexcrichton
Copy link
Member

I have added a note to our style guide about this https://github.com/mozilla/rust/wiki/Note-style-guide#iterators

flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 30, 2023
[`question_mark`]: don't lint inside of `try` block

Fixes rust-lang#8628.

Diff looks a bit noisy because I had to move the two functions into an impl, because they now need to access the structs `try_block_depth` field to see if they're inside a try block.

changelog: [`question_mark`]: don't lint inside of `try` block
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

Successfully merging this pull request may close these issues.