Skip to content

Provide an explicit iterator for SyntaxCollection #13112

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

Merged
merged 1 commit into from
Nov 28, 2017

Conversation

CodaFi
Copy link
Contributor

@CodaFi CodaFi commented Nov 28, 2017

The default IndexingIterator for a custom collection is not suitable for
subclasses that specialize the element type the way libSyntax does.
Overriding it with a more specific iterator, even with something like
covariant overrides, would not work because it the iterator is generic
over its base collection, not its element type.

Provide a custom iterator parameterized by the element type instead.

We discovered this while working with the libSyntax representation in Silt.

The default IndexingIterator for a custom collection is not suitable for
subclasses that specialize the element type the way libSyntax does.
Overriding it with a more specific iterator, even with something like
covariant overrides, would not work because it the iterator is generic
over its base collection, not its element type.

Provide a custom iterator parameterized by the element type instead.
@CodaFi CodaFi requested a review from harlanhaskins November 28, 2017 17:42
@CodaFi
Copy link
Contributor Author

CodaFi commented Nov 28, 2017

@swift-ci please smoke test

@harlanhaskins
Copy link
Contributor

I wouldn't say this is NFC, because it changes what the iterator type is, no?

Regardless, this is how this was intended to work. LGTM.

@CodaFi
Copy link
Contributor Author

CodaFi commented Nov 28, 2017

I wouldn't say this is NFC, because it changes what the iterator type is, no?

Before this patch, iterating over the syntax collection in a for-each loop wasn't possible. The only way clients would break is if they were relying on the default iterator type specifically being IndexingIterator.

@harlanhaskins
Copy link
Contributor

harlanhaskins commented Nov 28, 2017

Right, yes. But adding that functionality makes this an FC. 😉

@CodaFi CodaFi changed the title [NFC] Provide an explicit iterator for SyntaxCollection Provide an explicit iterator for SyntaxCollection Nov 28, 2017
@CodaFi
Copy link
Contributor Author

CodaFi commented Nov 28, 2017

Eh, I'm not willing to die on this hill.

Copy link
Contributor

@nkcsgexi nkcsgexi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@harlanhaskins
Copy link
Contributor

CI Passed, I say merge. ⛴

@harlanhaskins harlanhaskins merged commit cd64938 into swiftlang:master Nov 28, 2017
@CodaFi CodaFi deleted the reiterate branch November 28, 2017 21:15
maldahleh pushed a commit to maldahleh/swift that referenced this pull request Oct 26, 2020
The default IndexingIterator for a custom collection is not suitable for
subclasses that specialize the element type the way libSyntax does.
Overriding it with a more specific iterator, even with something like
covariant overrides, would not work because it the iterator is generic
over its base collection, not its element type.

Provide a custom iterator parameterized by the element type instead.
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.

3 participants