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

add new range constructors #1295

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

trws
Copy link
Member

@trws trws commented Jul 12, 2022

This is a draft of new range ergonomics. I did it for now as new overloads of make_range because we already have that, but I'm tempted to do something else with naming. The first of these was my original thought, but each has some benefits:

  • RAJA::range([start,]end[,stride])
  • RAJA::range::numeric(), this or others like it would let us have list() or similar as another option
  • RAJA::range::iota()

The big win here is that good old type inference and argument counts will give us well formed TypedRangeSegment and TypedRangeStrideSegment as appropriate with a short, rather pythonic, syntax. Trick is how to make it all fit in.

Comment on lines +537 to +540
* \return a newly constructed TypedRangeSegment where the
* value_type is equivilent to the common type of
* @begin and @end. If there is no common type, then
* a compiler error will be produced.
Copy link
Member

Choose a reason for hiding this comment

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

update comment

@MrBurmark
Copy link
Member

Do you want to support making ranges from iterators too?

@trws
Copy link
Member Author

trws commented Jul 12, 2022

Probably yes, I imagine we'll want a convenient way to get TypedListSegment and span instances, maybe even IndexSets. This is kinda step one to feel out how we'd want the interface to look, do you have a specific iterator use-case in mind?

@MrBurmark
Copy link
Member

I was thinking of taking pointer and length or pointer and pointer and producing spans.

@MrBurmark
Copy link
Member

I don't think we can do TypedListSegment if it still requires a resource.

@trws
Copy link
Member Author

trws commented Jul 12, 2022

We had recent discussions of making ListSegment un-owned only, which come to think of it would basically just make it a span wouldn't it? That's an interesting thought, maybe we should reconsider our naming and taxonomy a bit while we're at it.

template<typename Idx>
::RAJA::TypedRangeSegment<Idx> range(Idx start, Idx end, Idx stride)
{
return ::RAJA::TypedRangeStrideSegment<Idx>(
Copy link
Member

Choose a reason for hiding this comment

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

This isn't right.

@MrBurmark
Copy link
Member

MrBurmark commented Aug 3, 2022

Looking more closely at the Segment implementations, RangeSegment is basically the same as Span, it holds two iterators. RangeStrideSegment is similar, it also holds size so division is only done at construction. Should we consider condensing the implementation of the RangeSegments and Span into one type? In another PR.

@rhornung67
Copy link
Member

@MrBurmark I think that is the sort of thing we want to decide. IMO, condensing the types and implementations is good. We want to end up with something that is clear and intuitive for users.

@trws
Copy link
Member Author

trws commented Aug 3, 2022

In the end, we probably want them all to be "ranges", and "views" in the c++20 sense, which means a combination of an iterator and a sentinel, and in the case of a view it also means that move or copy construction is O(1). In principle the numeric ranges are the same as a span over an iterator, rather than a pointer. They could be the same thing as long as we keep the check logic in a creation function.

@trws trws force-pushed the scogland1/feature/range-ergonomics branch from a9bc401 to de633a0 Compare August 3, 2022 22:20
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