Skip to content

Conversation

@dukc
Copy link
Contributor

@dukc dukc commented Dec 11, 2017

…instead of enforcing. This is of course controversal since it's breaking but I decided it's worth a discussion. Isn't it after all clearly a bug if zipped ranges with StoppingPolicy.RequireSameLength have different lengths?

I made the same change for enforcement of !stoppingPolicy != StoppingPolicy.Longest for LockStep and enforcement of default-initializable elemets.

The advantages are that functions can be faster in production code and, in case of zip, nothrow can be inferred.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @dukc! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Copy link
Member

@MetaLang MetaLang left a comment

Choose a reason for hiding this comment

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

There's no way we can change this; it's very likely that there's client code out there that depends on zip/lockstep throwing an exception; therefore, it's wrapped in a try-catch that catches Exception. Changing this to throw an Error on length mismatch means that code will still compile but will now crash at runtime.

@dukc
Copy link
Contributor Author

dukc commented Dec 11, 2017

Such code is doubtlessly bug-prone anyway since the length property does not check for all ranges, resulting in overflow sooner or later. But that makes the code no less existent...

@JackStouffer
Copy link
Contributor

I can tell you from experience that Andrei will reject this due to its breaking nature, and I'd have to agree with him.

I would refactor this to add a new StoppingPolicy which gives this behavior.

@dukc
Copy link
Contributor Author

dukc commented Dec 11, 2017

Good idea, I think. Does not let us infer nothrow, though.

Already 3 to be considered against. Enough to convince me off.

@dukc dukc closed this Dec 11, 2017
@wilzbach
Copy link
Contributor

FWIW DIP1008 has been merged two days ago. So while you might not get nothrow, inference of @nogc due to Exceptions being @nogc might happen very soon ;-)

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.

5 participants