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

Revert fix for 18631. #6331

Merged
merged 1 commit into from Mar 23, 2018
Merged

Revert fix for 18631. #6331

merged 1 commit into from Mar 23, 2018

Conversation

jmdavis
Copy link
Member

@jmdavis jmdavis commented Mar 23, 2018

The fix for 18631 broke code. Ranges cannot be marked with const or
inout in generic code. The range API does not require that any functions
be const or inout. In this case, with the changes, choice requires that
length and opSlice be const, breaking any code that uses a range with
choice that does not have a const length or opSlice (which is going to
be a large percentage of ranges with those functions which aren't
arrays).

The fix for 18631 broke code. Ranges cannot be marked with const or
inout in generic code. The range API does not require that any functions
be const or inout. In this case, with the changes, choice requires that
length and opSlice be const, breaking any code that uses a range with
choice that does not have a const length or opSlice (which is going to
be a large percentage of ranges with those functions which aren't
arrays).
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @jmdavis!

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

@quickfur quickfur left a comment

Choose a reason for hiding this comment

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

Unfortunately, I have to agree. I don't see any easy way to salvage the original fix. Perhaps a separate overload for arrays might be good enough. But I don't see how it could be solved for general ranges, where the const-ness of methods are not guaranteed.

@n8sh
Copy link
Member

n8sh commented Mar 23, 2018

Unfortunately, I have to agree. I don't see any easy way to salvage the original fix. Perhaps a separate overload for arrays might be good enough. But I don't see how it could be solved for general ranges, where the const-ness of methods are not guaranteed.

Couldn't you have two overloads and explicitly check if length and opSlice are const, one overload requiring both and the other overload requiring not both?

@jmdavis
Copy link
Member Author

jmdavis commented Mar 23, 2018

Couldn't you have two overloads and explicitly check if length and opSlice are const, one overload requiring both and the other overload requiring not both?

The normal solution that we've gone with is to just do nothing about const. Normally, the code just works. The problem here is caused by auto ref, which we don't normally use with ranges, though it arguably makes sense here. The easiest solution is to just add an overload specifically for dynamic arrays, and in general, I think that it's just plain better to not do anything with const or inout with any function involving ranges. It's just a recipe for trouble. Ranges in general don't work with const and as soon as we try to add it here and there, we'll just get request after request to deal with one corner case or another without actually fixing the overall problem, because that can't be fixed. const and ranges simply do not work together due to how ranges are designed.

Regardless, I'd rather leave a proper fix to another PR and just revert the current changes so that the broken code is no longer broken.

@schveiguy
Copy link
Member

Copying comment from other PR:

Since it's pretty accepted that you can slice an object and get a range from it, Another way to fix this may be (not a full implementation, needs some tweaking):

auto ref choice(R)(auto ref R r) if (!isRandomAccessRange!R && isRandomAccessRange!(typeof(r[])))
{
    return choice(r[]);
}

It should work for containers as well, such as Array!T

@dlang-bot dlang-bot merged commit 4b166f3 into dlang:master Mar 23, 2018
@jmdavis jmdavis deleted the revert branch March 24, 2018 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants