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 std.range.indexed and chunks. #219

Merged
merged 6 commits into from Sep 1, 2011
Merged

Conversation

dsimcha
Copy link
Collaborator

@dsimcha dsimcha commented Aug 25, 2011

Round 2 after messing with Git a while. This pull request adds indexed and chunks to std.range. For details, see the docs in the source to the pull request.

@dsimcha
Copy link
Collaborator Author

dsimcha commented Aug 27, 2011

I added a .length property to transversal as a throw-in to this pull request, since it was a trivial fix and wasn't worth making its own branch for.

assert(chunks.length == 3);
---
*/
struct Chunks(Source) if(hasSlicing!Source && hasLength!Source)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think hasSlicing and hasLength is required for chunks. The operation can be done simply with input ranges. In generator form,

auto chunks(R)(R source, size_t n) if (isInputRange!R) {
    while (!source.empty) {
       (auto chunk, source) = adjoin!(take, drop)(source, n);
       yield chunk;
    }
}

(and there should be a function for computing take and drop in one go.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, this is definitely do-able and a good idea for forward ranges. It would even come without any significant loss of efficiency because take and drop/popFrontN both use slicing if supported. I can't see how it would work for input ranges, though. Chunks.front would return a Take!(SomeInputRange). Popping the front of it would also pop the source range that the Chunks struct is holding onto. What you'd really need to do is:

void popFront() {   
    popFrontN(source, _chunkSize);
}

auto front() @property {
    return take(source.save, _chunkSize);
}

}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On second thought, I'm concerned about the severe hidden inefficiency here. If popFrontN/drop is not an O(1) operation, using chunks will force this expensive operation to be done twice: Once when popping off the returned chunk and once when popping off the source range to get to the next chunk. Furthermore, without length, bidirectional iteration is impossible because you wouldn't know how long the last chunk should be.

@dsimcha
Copy link
Collaborator Author

dsimcha commented Aug 30, 2011

Ping?

@dnadlinger
Copy link
Member

Looks good to me – I'd definitely expect something like indexed and chunk to be in Phobos. Regarding the hasSlicing requirement, couldn't support for non-sliceable ranges just transparently be added later?

@dsimcha
Copy link
Collaborator Author

dsimcha commented Aug 30, 2011

Yes, support for non-sliceable could be added later w/o breaking any code relying on the current version. It's not difficult to implement for forward ranges. It's not in there now because, as I mention, I'm skeptical of its net benefits and its ability to hide lots of inefficiency.

assert(equal(ind, [5, 4, 2, 3, 1, 5]));

// When elements of indices are duplicated and Source has lvalue elements,
// these are aliased in reindexed.
Copy link
Member

Choose a reason for hiding this comment

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

While I actual preferred the names Reindex and reindexed, if you're going to change them, then this comment needs to be fixed so that it doesn't mention reindexed.

@jmdavis
Copy link
Member

jmdavis commented Sep 1, 2011

Aside from a couple of nitpicks that shouldn't take long to address, this looks to me like it's ready to be merged in.

@dsimcha
Copy link
Collaborator Author

dsimcha commented Sep 1, 2011

Thanks for pointing these out. Fixed.

@jmdavis
Copy link
Member

jmdavis commented Sep 1, 2011

Merging.

jmdavis added a commit that referenced this pull request Sep 1, 2011
Add std.range.indexed and chunks.
@jmdavis jmdavis merged commit d0df3f1 into dlang:master Sep 1, 2011
kuettler pushed a commit to kuettler/phobos that referenced this pull request Feb 6, 2018
fix issue 17198 - rdmd does not recompile when --extra-file is added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants