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

Provide concept emulation for Ranges-TS concepts #3684

Merged
merged 11 commits into from Feb 23, 2019

Conversation

Projects
None yet
3 participants
@apmccartney
Copy link
Contributor

commented Feb 10, 2019

This pull request provides concept emulation for Ranges-TS bidirectional and random access iterator concepts.

This is still a work in progress, but I generally prefer to iterate with library authors when developing a contribution.

I believe I've conformed the style guidance in the contributing guidelines. I've also attempted to emulate the local coding style for the file I've modified (but I would be surprised if I inferred the conventions perfectly). In either case, I'm happy to make any stylistic corrections.

Regarding the type traits defined in this submission, I was torn as to whether they belonged in the hpx/trait/is_iterator header or in dedicated files in the hpx/util direction. For the moment, I have opted for the former. Direction in this respect would be welcome.

It appears there is no unit testing for the is_iterator header. I'm building and running the complete test suite at present, but am willing to provide unit tests to demonstrate the added functionality in isolation.

Proposed Changes

  • Add relevant type traits underlying the BidirectionalIterator and RandomAccessIterator concepts
    • addition_result
    • dereference_result
    • equality_result
    • inequality_result
    • inplace_addition_result
    • inplace_subtraction_result
    • predecrement_result
    • preincrement_result
    • postdecrement_result
    • postincrement_result
    • subscript_result
    • subtraction_result
  • Add a type trait to verify whether a traversal concept is satisfied
    • satisfy_traversal_concept
  • Add specializations to has_traversal and belongs_to_iterator_traversal integrating the concept check.

Any background context you want to provide?

See #3641

@hkaiser
Copy link
Member

left a comment

Do we still need the explicit handling of the boost traversal categories with the new traits?

@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

Regarding the type traits defined in this submission, I was torn as to whether they belonged in the hpx/trait/is_iterator header or in dedicated files in the hpx/util direction. For the moment, I have opted for the former. Direction in this respect would be welcome.

Having the new traits in that header should be good enough. I don't think those are of any use outside of iterator categorization.

It appears there is no unit testing for the is_iterator header. I'm building and running the complete test suite at present, but am willing to provide unit tests to demonstrate the added functionality in isolation.

Tests would be very welcome, indeed.

Thanks for your work on this!

@apmccartney

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2019

Do we still need the explicit handling of the boost traversal categories with the new traits?

In short, for the BidirectionalIterator and RandomAccessIterator, no, and for weaker iterators, yes.

For BidirectionalIterator and RandomAccessIterator, the interface guarantees are sufficient to robustly distinguish types implementing the two concepts. The boost traversal categories really only provide a mechanism to 'lie', i.e. specify a iterator level that is stronger or weaker than your interface would otherwise suggest (which is probably a footgun). However, the situation with ForwardIterator and InputIterator is less straight forward.

Fundementally, the multiple pass guarantee is what distinguishes the ForwardIterator concept from the InputIterator concept in the ranges TS. To my knowledge, there is no means to deduce this from the iterator interface; we really do need author annotations of some sort to make a determination. Prior to the C++ 20 standard and its various iterator category requirement reductions, the traversal categories of the boost iterator library are the best way that I know of to do so.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

The remaining test failures are unrelated.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

@apmccartney IIUC, a user still needs to specialize the Boost traversal category for their iterators if those are input iterators. Is that correct?

@apmccartney

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

A user still needs to specialize the Boost traversal category for their iterators if those are input iterators. Is that correct?

If the user's iterator models forward traversal and nothing stronger, then yes, the user must specialize boost::iterator::iterator_traversal on their iterator type in order for hpx algorithms to take advantage of that forward traversal gaurantees.

If the user's iterator models bidirectional traversal or stronger, then no, the user need not take any action for hpx algorithms to take advantage of that traversal (regardless of the iterator's nested iterator_category typedef).

@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

If the user's iterator models bidirectional traversal or stronger, then no, the user need not take any action for hpx algorithms to take advantage of that traversal (regardless of the iterator's nested iterator_category typedef).

@apmccartney Thanks for this explanation. However, when I use the reduce_iterators branch (see #3650), merge your changes there, and edit this line to input_iterator_tag, I still see compilation problems stating that the used iterator is not at least a forward iterator. I think the iterator used in that test exposes the interface of a random access traversable iterator, so things should compile. Would you mind having a look?

@apmccartney

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

However, when I use the reduce_iterators branch (see #3650), merge your changes there, and edit this line to input_iterator_tag, I still see compilation problems stating that the used iterator is not at least a forward iterator.

There's apparently a bug in my implementation. Testing to come should sort that out. =)

Would you mind having a look?

Absolutely. I'll take a look this evening.

EXPAND CONCEPT CHECKING FOR FORWARD ITERATORS
This commit adds a specialization to the `satisfy_traversal_concept` for
the `boost::forward_traversal_tag` provided by the `boost::iterator`
library.

Forward traversal cannot be robustly verified via concept emulation due
to the ambiguities between iterfaces specified by the `ForwardIterator`
and `InputIterator` concepts in the C++20 ranges TS. That said, any
class that satisfies the `BidirectionalIterator` concept is certain to
also fullfill the `ForwardIterator` concept.

The specialization established in this commit leverages that fact to
provide partial concept checking for `ForwardIterator`.
@apmccartney

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

I reviewed the compilation failure for reduce_3641.cpp after merging my branch and making the perscribed modications to the file. After a bit of poking around, I was pleased to find that my implementation was simply incomplete rather than seriously flawed. I've made the corresponding extension and the test is now passing on my locally merged branch.

The type trait assertion on reduce required the argument iterator at least modeled a forward traversal. This pull request defines that to mean that either

  1. the boost::iterator::iterator_traversal<T>::type was boost::forward_traversal_tag or a child class thereof or
  2. satisfy_traversal_concept<T, boost::forward_traversal_tag>::value was true.

In general, it is not possible to determine whether a given iterator with the forward traversal interface models a forward iterator or an input iterator. However, there is a special case in which that determination could be made. The concept checking was extended to take avantage of that circumstance. I've explained in some detail in the commit message.

... any class that satisfies the BidirectionalIterator concept is certain to also fullfill the ForwardIterator concept.

The specialization established in this commit leverages that fact to provide partial concept checking for ForwardIterator.

I plan to take some time Saturday afternoon to complete a small set of unit tests. Is there anything else you would like to see as part of this merge request?

@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Is there anything else you would like to see as part of this merge request?

@apmccartney Could you add some comment to the specialization of satisfy_traversal_concept<Iter, boost::forward_traversal_tag> (https://github.com/STEllAR-GROUP/hpx/pull/3684/files#diff-ad7521def89116999da36bcdfd269fbdR304) explaining your decision, please? A casual reader might otherwise not understand why it is derived from bidirectional_concept<Iter> (frankly, my first thought was: 'This must be a typo!'). Otherwise, all is fine and good to go. Thanks!

Add comment for forward traversal specialization
The special case of `satisfy_traveral_concept` for forward traversal
is counter intuitive and correctly identifies only a subset of the types
satisfying the true concept requirements. A comment was added to explain
why this is the case.
@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

@apmccartney Some of our algorithm tests fail as a result of your changes (see https://circleci.com/gh/STEllAR-GROUP/hpx/67315 and https://circleci.com/gh/STEllAR-GROUP/hpx/67327). Would you be able to have a look?

@apmccartney

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Yes, absolutely. Looking now. I apologize for the delayed response. Recovering from illness.

apmccartney added some commits Feb 21, 2019

Added unit testing for trait/is_iterator header
Thes tests provided in this commit are not formatted to the style conventions
found in the the larger hpx codebase, but serve as a reasonable starting point
for investigating test failures.

@hkaiser hkaiser force-pushed the apmccartney:master branch from 7dfad0a to 240135e Feb 21, 2019

Fixing iterator_facade to properly implement the post-increment operator
- flyby: (clang-)formatting new code related to is_iterator
- flyby: add more tests verifying the changes

@hkaiser hkaiser force-pushed the apmccartney:master branch from 240135e to 1635960 Feb 21, 2019

@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

@msimberg this is good to go. The errors on pycicle are unrelated, afaics.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

@msimberg I would like to merge this asap in order to get #3641 and #3650 out of the way.

@msimberg

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

@hkaiser almost there... @apmccartney unfortunately we still try to support C++11 (although we've discussed dropping it soonish). That means your autos for return types won't work, see here. Would you mind fixing those?

@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

@hkaiser almost there... @apmccartney unfortunately we still try to support C++11 (although we've discussed dropping it soonish). That means your autos for return types won't work, see here. Would you mind fixing those?

I'll fix those. Thanks!

@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

@msimberg should be fine now, let's see how the tests go.

@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

@msimberg things look good now, I believe. The C++11 builds have succeeded. The tests fail at runtime because of a configuration error in pycicle… Merging now.

@hkaiser hkaiser merged commit 33b440a into STEllAR-GROUP:master Feb 23, 2019

10 of 16 checks passed

pycicle daint-clang-3.8-boost-1.58.0-c++11-Release Test errors 631
Details
pycicle daint-clang-7.0-boost-1.68.0-c++17-nonetworking-Debug Test errors 1
Details
pycicle daint-gcc-7.3.0-boost-1.68.0-c++17-Release Build errors 11
Details
pycicle daint-gcc-7.3.0-boost-1.68.0-c++17-Release Test errors 1
Details
pycicle daint-gcc-7.3.0-cuda-9.2.148_3.19-6.0.7.1_2.1__g3d9acc8-boost-1.68.0-c++11-Release Build errors 14
Details
pycicle daint-gcc-7.3.0-cuda-9.2.148_3.19-6.0.7.1_2.1__g3d9acc8-boost-1.68.0-c++11-Release Test errors 2
Details
build-and-test Workflow: build-and-test
Details
pycicle daint-clang-3.8-boost-1.58.0-c++11-Release Build errors 0
Details
pycicle daint-clang-3.8-boost-1.58.0-c++11-Release Config errors 0
Details
pycicle daint-clang-7.0-boost-1.68.0-c++17-nonetworking-Debug Build errors 0
Details
pycicle daint-clang-7.0-boost-1.68.0-c++17-nonetworking-Debug Config errors 0
Details
pycicle daint-gcc-4.9.3-boost-1.58.0-c++11-Debug Build errors 0
Details
pycicle daint-gcc-4.9.3-boost-1.58.0-c++11-Debug Config errors 0
Details
pycicle daint-gcc-4.9.3-boost-1.58.0-c++11-Debug Test errors 0
Details
pycicle daint-gcc-7.3.0-boost-1.68.0-c++17-Release Config errors 0
Details
pycicle daint-gcc-7.3.0-cuda-9.2.148_3.19-6.0.7.1_2.1__g3d9acc8-boost-1.68.0-c++11-Release Config errors 0
Details
@hkaiser

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

@apmccartney Congrats to you for your first accepted PR into HPX. Please get in contact with @aserio if you're interested in receiving a STE||AR-Group t-shirt. We give that away for all of our first-time contributors.

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.