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

Fix Issue 19289 - std.range.transposed with enforceNotJagged not throwing #6723

Merged
merged 1 commit into from Oct 6, 2018
Merged

Fix Issue 19289 - std.range.transposed with enforceNotJagged not throwing #6723

merged 1 commit into from Oct 6, 2018

Conversation

ghost
Copy link

@ghost ghost commented Sep 30, 2018

@dlang-bot
Copy link
Contributor

dlang-bot commented Sep 30, 2018

Thanks for your pull request and interest in making D better, @crocopaw! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
19289 normal std.range.transposed with enforceNotJagged not throwing

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#6723"

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

This needs to come with a unittest that assertThrown.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM, assuming no changelog is needed otherwise open an issue and reference it in the git commit, see guide i don't remember where.


if (empty) return;
immutable commonLength = _input.front.length;
foreach (e; _input)
Copy link
Member

Choose a reason for hiding this comment

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

_input.front's length is unnecessarily enforced to be equal to itself.

Copy link
Author

Choose a reason for hiding this comment

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

I just copied the code from transverse. If you've got an idea how to remove this redundancy you should do this at transverse as well.

Copy link
Member

Choose a reason for hiding this comment

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

Fair.

@n8sh
Copy link
Member

n8sh commented Oct 1, 2018

Linux_64_32 failure appears unrelated.

@n8sh
Copy link
Member

n8sh commented Oct 1, 2018

Add a reference to an issue number and this can be merged.

@n8sh n8sh changed the title Added implementation for transposed for enforceNotJagged. Fix Issue 19289 - std.range.transposed with enforceNotJagged not throwing Oct 6, 2018
@n8sh n8sh added the auto-merge label Oct 6, 2018
@dlang-bot dlang-bot merged commit 264ab21 into dlang:master Oct 6, 2018
@ghost ghost deleted the transposed branch October 7, 2018 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants