Skip to content

[opt-transform-range] Do not assume that Range::iterator exists. #27051

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

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Sep 6, 2019

The reason not to do this is that some important range
adapters (e.x. llvm::iterator_range) do not have such a typedef. Rather than
changing upstream this commit just fixes the problem by inferring the iterator
type from the result of Range::begin().

TLDR: This makes OptionalTransformRange work with llvm::iterator_range.

The reason not to do this is that some important range
adapters (e.x. llvm::iterator_range) do not have such a typedef. Rather than
changing upstream this commit just fixes the problem by inferring the iterator
type from the result of Range::begin().

TLDR: This makes OptionalTransformRange work with llvm::iterator_range.
@gottesmm
Copy link
Contributor Author

gottesmm commented Sep 6, 2019

@swift-ci smoke test

@atrick atrick self-requested a review September 6, 2019 16:02
Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

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

I spent a good hour trying to work around this yesterday. My approach was to move to llvm::map_range (in the next version of LLVM). Unfortunately, mapped_range has a bug because it doesn't propagate the return type correctly. So this is a good stop-gap.

@gottesmm gottesmm merged commit 681a203 into swiftlang:master Sep 6, 2019
@gottesmm gottesmm deleted the pr-e2c4cb6f4381f6d58f10ec9729cadfb4407574df branch September 6, 2019 16:16
@gottesmm
Copy link
Contributor Author

gottesmm commented Sep 6, 2019

@atrick once those improvements land in LLVM, we should start ripping out our own custom stuff.

@atrick
Copy link
Contributor

atrick commented Sep 6, 2019

@gottesmm Yeah, I have it on my TODO list to fix llvm::mapped_range so we can use it

@gottesmm
Copy link
Contributor Author

gottesmm commented Sep 6, 2019

@atrick we also should upstream something like optional transform range (that is something that maps -> optional and then filters).

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.

2 participants