Update in chunks #992
Update in chunks #992
Conversation
| /// Length. Only if $(D hasLength!Source) is $(D_PARAM true) | ||
| @property size_t length() | ||
| { | ||
| return cast(size_t)((_source.length + _chunkSize - 1UL) / _chunkSize); |
There was a problem hiding this comment.
Oh, and the cast isn't needed either - it's all size_ts. No?
There was a problem hiding this comment.
The ˋULˋ is a trick to force promotion to ulong, in case of overflow, before doing the division. Then the cast is to downcast back to size_t. I should have definitly commented it, and used a clearer upcast, too. Sorry about that.
Come to think about it, the upcast would probably be too late here anyways... I'll just change it to a straight up:
ˋcast(ulong)_source.length + ...ˋ
There was a problem hiding this comment.
Definitely comment it if you're trying to do anything like that, and yes, any overflow would have already occurred when 1UL was reached.
|
lgtm modulo nits |
| static if (isInfinite!Source) | ||
| // undocumented | ||
| enum empty = false; | ||
| else | ||
| /// Ditto |
There was a problem hiding this comment.
This comment isn't going to show up in the docs. It's the first static if which wins inside of a template. So, you should move the finite version above the infinite one.
|
Ok. I reviewed and addressed all the above comments. I also fixed an issue with the slicing operator that failed on operations such as The second commit contains something which (IMO) is a very interesting mechanic: I defined opDollar to return a typed dollartoken. This means that even when chunks is finite, you can still bypass a ton of code by statically telling it you want to slice to end. This is especially interesting when combined with I think it might be better to review commits individually. The first commit is the "meat" commit. The second is really just cherry on the cake. If it is deemed overkill, we can remove it. EDIT: I had to makes changes that merged the two commits. Sorry :( |
|
OK: Everything is done, and all prerequired commits have been pulled, and I've rebased after them. Primary objective: Make chunks work with an input range: Done. Also, chunks is now able to correctly wrap an infinite sliceable range, and provide efficient slicing and slice to end of said range! How cool is that? Non-infinite ranges can now also efficently slice to $, using a static dollar type. This is important because in the case of chunks, there is a fair amount of computation to calculate length, and then to re-transform an index the the source's index. Fixed issues where chunks[$ .. $] used to crash. So, just to say this is done and ready for review. |
|
Fails the auto tester. |
|
Just a quick question. Was "alias this = Something;" made illegal? I know there was talks about it, and I'm getting compiler complaints around it. |
|
Yes. Use |
|
Hum, I'm getting a weird bug. May or may not be my fault, but I'm getting some real erratic behavior: basically, a In particular, it is produced by one of (line aprox 6700): assert (chunks1[$ .. 5].equal(empy)); //Semiquick
assert (chunks2[$ .. 5].equal(empty)); //SemiquickBut if I comment either of them, than neither individually produces the bug. Also, if I change the order of the "equal" args, then it doesn't produce the bug: assert (empty.equal(chunks1[$ .. 5])); //Semiquick
assert (empty.equal(chunks2[$ .. 5])); //SemiquickI'm still investigating on the grounds that it is probably my fault, but I'm reporting it in case it smells stinky to anyone... That and I have no idea how an opDollar could produce a null this :/ |
|
Well, I think I'm hitting a codegen/compiler bug or something. I'll reduce the test case and submit it. |
|
Yeah, it is a brand new regression. From the 8th. Somebody should look into it while it is fresh: |
| } | ||
| else | ||
| { | ||
| return _source[len - remainder..len]; | ||
| //Dollar token caries a static type, with no extra information. | ||
| //It can lazilly transform into _source.length on algorithmic |
There was a problem hiding this comment.
Typos: »caries«, »lazilly«. Not that it would matter much in a comment.
reduces requirements to ForwardRange
|
Are we good to go? |
|
Appart from an outstanding typo in a comment that I just fixed, it should be ready to go, yes. |
|
Yay! Thanks :) |
Basically the old
if(isInputRange!Source && hasSlicing!Source && hasLength!Source)was pretty much the same as "finite RA".This implementation only requires
ForwardRange. This meanschunkscan now be used with "simple" ranges, such asbyLine(to read blocks of lines at once), orfilter, or infinite ranges, both non-ra (Fib by blocks), or with RA and slicing (Sequence).AFAIK, chunks requires at least forward: I don't think it could work on a input only.
Implementation wise, there is not much: slicing was basically just changed for
take, and everything works just as well. I re-organized a bit so as to document what functions require what traits fromSource. Finally, I improved on the implementations ofBack/popBack/Length, simply by using more accurate arithmetics.This pull currently works, but would require #854 to fully unlock the slicing functionality of infinite ranges, and requires #991 so as to test said functionality.