Skip to content

[cxx-interop] Add a basic test for using std::iterator_traits #41290

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

cabmeurer
Copy link
Contributor

@cabmeurer cabmeurer commented Feb 9, 2022

Add basic tests for using std::iterator_traits

Ref: #41166

@cabmeurer cabmeurer force-pushed the cxx/add-stdlib-iterator_traits-test branch 3 times, most recently from d91b9c8 to 594f464 Compare February 9, 2022 03:15
@cabmeurer
Copy link
Contributor Author

cabmeurer commented Feb 9, 2022

@hyp
@zoecarver
I thought I would give it a go and try to add some tests for iterator_traits

@cabmeurer cabmeurer changed the title [cxx-interop] Add a basic test for using stdlib iterator_traits [cxx-interop] Add a basic test for using std::iterator_traits Feb 9, 2022
@cabmeurer cabmeurer force-pushed the cxx/add-stdlib-iterator_traits-test branch 2 times, most recently from bdb11fd to 265b8f2 Compare February 9, 2022 03:34
@hyp
Copy link
Contributor

hyp commented Feb 10, 2022

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 265b8f229daded8609875d85d73f6fce60ac8920

@cabmeurer cabmeurer force-pushed the cxx/add-stdlib-iterator_traits-test branch from 265b8f2 to 3945f47 Compare February 11, 2022 00:43
@cabmeurer
Copy link
Contributor Author

@swift-ci please test

@cabmeurer
Copy link
Contributor Author

@zoecarver when you get a chance, could you merge this for me?

Copy link
Contributor

@zoecarver zoecarver left a comment

Choose a reason for hiding this comment

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

I'm sorry, I don't think that this really makes sense to test. If we are able to construct an object, it won't be nil. It might be interesting to see if we can import iterator_traits (a module interface test) but honestly, I'm not even sure how valuable that would be. I don't think people will be using traits like this very often in Swift.

What would be really helpful is some tests for std::next and std::prev (if you have time, std::distance, and std::advance might be interesting too) because I anticipate those will be used very frequently. Could you update this PR to test those instead (or make a new PR)?

I really appreciate you spending time writing tests! So, I'm sorry if this is disappointing :(

@zoecarver
Copy link
Contributor

I should also mention that these tests look 100% correct, so really well done for your first PR! It's just the thing you're testing doesn't really make sense.

@cabmeurer
Copy link
Contributor Author

I'm sorry, I don't think that this really makes sense to test. If we are able to construct an object, it won't be nil. It might be interesting to see if we can import iterator_traits (a module interface test) but honestly, I'm not even sure how valuable that would be. I don't think people will be using traits like this very often in Swift.

What would be really helpful is some tests for std::next and std::prev (if you have time, std::distance, and std::advance might be interesting too) because I anticipate those will be used very frequently. Could you update this PR to test those instead (or make a new PR)?

I really appreciate you spending time writing tests! So, I'm sorry if this is disappointing :(

Thank you! All good, excited that i can contribute and get feedback, it's the best way to learn. I completely follow and I will spend some time adding tests for std::next std::prev std::distance and std::advance. As thats also what we talked about on #41232

Thank you for taking the time to go over this stuff with me! I really appreciate it

@cabmeurer cabmeurer closed this Feb 18, 2022
@cabmeurer cabmeurer deleted the cxx/add-stdlib-iterator_traits-test branch February 18, 2022 23:45
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.

4 participants