Bugfix for takeOne (issue 16999) by dukc · Pull Request #4984 · dlang/phobos

dukc · 2016-12-21T16:01:31Z

Fixed a bug: takeOne with unslicabe ranges did not pop its source range when popped. This made it inconsistent with take and takeExactly when using reference ranges. Also corrected its documentation.

wilzbach

Good catch! Two general remarks:

When touching uncovered code, it's considered a good style to provide a unittest for it.
It would be nice if you could file an issue at Bugzilla and reference it in your commit message, s.t. our bot can do it's work and for example include the fix in the changelog for the next release. The bot uses the same issue reference syntax as Bugzilla does. See other PRs, e.g. this one for examples.

wilzbach · 2016-12-21T16:19:22Z

std/range/package.d

            void popBack()
            {
                assert(!empty, "Attempting to popBack an empty takeOne");
+                _source.popFront();


This should be popBack and it's not even covered :/
(please add a test)

Wrong! I almost introduced a bug by doing first making it popBack(), but luckily some unittest didn't compile.

Think what happens if I takeOne from, say a refrange which contains [1, 2, 3, 4] and then popBack the takeOne. The range would become [1, 2, 3] even when its 1 i took from it.

You're correct about that testing thing trough

I aliased popBack to popFront. They do the same thing so this perhaps removes some bloat. At the same time it should fix that testing thing.

wilzbach · 2016-12-21T16:25:14Z

std/range/package.d

 regardless of $(D R)'s capabilities (another feature that distinguishes
-$(D takeOne) from $(D take)).
+$(D takeOne) from $(D take)). One exception: The range can save() only
+if $(D R) can.


How about using the term ForwardRange, e.g.

"However, it is a ForwardRange if range is forwardable."

(I am not sure whether this makes it clearer)

It does not: a random access range should always be a forward range, and takeOne clains to be a random access range (and thus ForwardRange) no matter what.

a random access range should always be a forward range

This is not necessarily required by the definition.

Hehe, check definition of isBidirectionalRange :-)

So I thought a bit about this and technically the status quo for takeOne is:

If only provided with an InputRange, takeOne is neither a RandomAccess nor a ForwardRange

but the spec also says:

r.front evaluated multiple times, without calling r.popFront, or otherwise mutating the range object or the underlying data, yields the same result for every evaluation.

So before your PR it would have been possible to provide save even for a InputRange, because a call to front is always guaranteed to yield the same element and the range can be "saved" as currently by storing it's emptiness, the source range and not calling popFront on the source range.

The spec only has been updated to this tight definition quite recently (since 2.072), before it was allowed to yield different results for multiple calls on front and e.g. most famously generate did so.

CC @JackStouffer and @schveiguy (the authors of this change)

Same conclusion. I clarified the thing in doc.

dukc · 2016-12-27T09:38:27Z

Too hard for me. I can't use Vim controls at all... were I to mistype a single key, I would not know what to do.

Is there a non-interactive way to rename commits?

PetarKirov · 2016-12-27T09:45:50Z

@dukc Then just change the editor, if you don't like vim ;)

https://git-scm.com/book/tr/v2/Customizing-Git-Git-Configuration
git config --global core.editor [editor exe name]

E.g. git config --global core.editor subl (if you use Sublime Text).

wilzbach · 2016-12-27T10:04:04Z

Is there a non-interactive way to rename commits?

Two other possibilities:

You can also use one of the GUI editors
I can also do the rebase for you - just let me know if you want to go this way

dukc · 2016-12-27T10:08:22Z

I had already changed the editor -- I just did not except that other editors could do interactive mode. Thanks for the info.

wilzbach · 2016-12-27T10:12:00Z

I just did not except that other editors could do interactive mode

@dukc now you seem to have added a lot more commits. Maybe a simple git rebase upstream/master will do?

dukc · 2016-12-27T10:20:44Z

i have a feeling i am screwing the whole thing up just trying to rename a single commit...

dukc · 2016-12-27T10:24:34Z

No, I can't get it to work now... I edit the message, save it and then get a merge problem or something. Too much new stuff to handle at once. I aborted the rebase.

dukc · 2016-12-27T10:35:04Z

I somehow managed to apparently include everything that has happened in the master branch recenty in the pr... I believe I have to close this and do the whole pr again?

wilzbach · 2016-12-27T10:45:22Z

I somehow managed to apparently include everything that has happened in the master branch recenty in the pr... I believe I have to close this and do the whole pr again?

Nope - fixed it for you ;-)

dukc · 2016-12-27T10:53:10Z

Thanks. I'm not going to be trying to rebase anything I've made pr's of anytime soon.

dukc · 2017-01-02T13:08:57Z

What's that CircleCI failure about... "reference is not a tree" sounds like a checker bug?

dukc · 2017-01-02T17:00:31Z

Please explain the above, pull or close. I messed up my private fork but I dare not to try to clean it up as long as this pr is based from it. That means my possible further work is blocked by this in the queue.

dukc · 2017-01-04T09:34:24Z

@wilzbach I will close this pr if no-one has anything to say about it anymore. Post above explains why.

wilzbach · 2017-01-04T12:58:50Z

What's that CircleCI failure about... "reference is not a tree" sounds like a checker bug?

Yep, I think it was due to this fixed problem. I rebased your PR and hopefully everything should be green now.
Also note that passing CircleCi is not enforced nor required, it's a mere tool to help reviewers.

Please explain the above, pull or close. I messed up my private fork but I dare not to try to clean it up as long as this pr is based from it.

Hmm - you just need to reset your master branch. As long as you don't delete your fork or the takeOneFix branch nothing should be affected.

That means my possible further work is blocked by this in the queue.

Don't worry about - in the worst case - GitHub makes copies of all PRs and I have them on my machines, so I could easily recreate this PRs.

@wilzbach I will close this pr if no-one has anything to say about it anymore. Post above explains why.

Sorry, I was away for a bit. Please don't let yourself get blocked by this. As said we will manage to find a way ;-)

dukc · 2017-01-04T13:20:31Z

Thanks, that means I can continue with other stuff, if I more find things fit for me.

JackStouffer

Looks good, just address the nitpicks

JackStouffer · 2017-01-11T08:09:47Z

std/range/package.d

    assert(s.back == 43);
    assert(s[0] == 43);
-    s.popFront();
+    s.popBack();


no reason to change this

Just wanted to make sure my alias works. Do you want me to put it elsewhere instead?

JackStouffer · 2017-01-11T08:11:56Z

std/range/package.d

+        @safe void popFront(){front++;}
+        enum empty = false;
+    };
+    auto iotaPart = myIota.takeOne;


auto iotaPart = iota(4);?

Won't do, because the bug affects only RefRanges.

dukc · 2017-01-13T19:35:28Z

Finally, fixed that fork! Trough, the lesson with pushing and pulling appears to be that if something is rejected, force it... usually that's a bad sign.

edi33416 · 2017-01-13T22:33:06Z

A short note on why you need to force a push to a remote branch. I hope you will find this helpful.

After you've rebased your local branch, you will have to force the push to your remote branch. That is because git rebase rewrites your commits history and you want to force the rewrite to the remote branch as well. If you don't force, the push will be rejected because of a mismatch in the commits history.

When you pull from a remote branch, it is probably a good idea to use the --ff-only option. The --ff-only option is to ensure that your local branch is identical to the remote branch, since otherwise you will end up with a very messy history that will be hard to clean up.

I have found rebasing explained really well (imho) here.

Hope this helps.
Cheers

dukc · 2017-01-14T07:35:34Z

Thanks, perhaps it will help if/whn i encounter more problems. My problems probably were due to thinking that if I'm forcing, I'm doing something wrong.

dukc · 2017-01-26T17:20:36Z

@JackStouffer Both nitpicks addressed. But are they ok now?

JackStouffer · 2017-01-26T18:49:14Z

Please squash and push: https://wiki.dlang.org/Starting_as_a_Contributor#Squashing

…ge when popped. This made it inconsistent with take and takeExactly when using reference ranges. Also corrected its documentation.

dukc · 2017-01-26T21:12:32Z

Thanks. Done, hopefully without complications.

JackStouffer · 2017-01-26T21:27:54Z

Auto-merge toggled on

dukc · 2017-01-26T21:53:57Z

Oh no, I screwed something with squashing, popBack is not aliased to popFront anymore. That's a secondary thing trough. Perhaps i'll do a separate pr about that.

wilzbach reviewed Dec 21, 2016

View reviewed changes

dukc changed the title ~~Bugfix for takeOne~~ Bugfix for takeOne (issue 16999) Dec 22, 2016

wilzbach mentioned this pull request Dec 24, 2016

Add explanatory README dlang/dlang-bot#37

Merged

wilzbach force-pushed the takeOneFix branch from da8b152 to b154220 Compare December 27, 2016 10:44

wilzbach force-pushed the takeOneFix branch from b154220 to d1ee99f Compare January 4, 2017 12:51

JackStouffer suggested changes Jan 11, 2017

View reviewed changes

JackStouffer reviewed Jan 11, 2017

View reviewed changes

dukc force-pushed the takeOneFix branch from d1ee99f to 5bf442f Compare January 13, 2017 19:31

JackStouffer approved these changes Jan 26, 2017

View reviewed changes

JackStouffer added the auto-merge-squash label Jan 26, 2017

JackStouffer removed the auto-merge-squash label Jan 26, 2017

Fixed a bug: takeOne with unslicabe ranges did not pop its source ran…

a520807

…ge when popped. This made it inconsistent with take and takeExactly when using reference ranges. Also corrected its documentation.

dukc force-pushed the takeOneFix branch from cd52e12 to a520807 Compare January 26, 2017 21:10

JackStouffer merged commit d60f32c into dlang:master Jan 26, 2017

dukc deleted the takeOneFix branch January 27, 2017 09:05

Uh oh!

Conversation

dukc commented Dec 21, 2016

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dukc Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dukc Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dukc commented Dec 27, 2016

Uh oh!

PetarKirov commented Dec 27, 2016

Uh oh!

wilzbach commented Dec 27, 2016

Uh oh!

dukc commented Dec 27, 2016

Uh oh!

wilzbach commented Dec 27, 2016

Uh oh!

dukc commented Dec 27, 2016

Uh oh!

dukc commented Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dukc commented Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Dec 27, 2016

Uh oh!

dukc commented Dec 27, 2016

Uh oh!

dukc commented Jan 2, 2017

Uh oh!

dukc commented Jan 2, 2017

Uh oh!

dukc commented Jan 4, 2017

Uh oh!

wilzbach commented Jan 4, 2017

Uh oh!

dukc commented Jan 4, 2017

Uh oh!

JackStouffer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dukc Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach Dec 21, 2016 •

edited

Loading

dukc Dec 21, 2016 •

edited

Loading

wilzbach Dec 21, 2016 •

edited

Loading

dukc Dec 21, 2016 •

edited

Loading

dukc commented Dec 27, 2016 •

edited

Loading

dukc commented Dec 27, 2016 •

edited

Loading

dukc Jan 13, 2017 •

edited

Loading

dukc commented Jan 13, 2017 •

edited

Loading

dukc commented Jan 26, 2017 •

edited

Loading