-
-
Notifications
You must be signed in to change notification settings - Fork 706
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
SortedRange.opSlice() should be return scope #6866
Conversation
|
Thanks for your pull request, @WalterBright! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf 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#6866" |
87aa33b to
d6b4093
Compare
|
buildkite/phobos fails with: What does that mean? |
|
Blocking dlang/dmd#9220 |
|
No, the error is: We went throw this numerous times. tl;dr: it's safe to merge (in terms of this particular Buildkite error) |
| @@ -10493,7 +10493,7 @@ if (isInputRange!Range && !isInstanceOf!(SortedRange, Range)) | |||
|
|
|||
| /// Ditto | |||
| static if (hasSlicing!Range) | |||
| auto opSlice(size_t a, size_t b) | |||
| auto opSlice(size_t a, size_t b) return scope @trusted | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the compiler be able to infer the safety here automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's more, Range.opSlice might be unsafe. This can't be @trusted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's more,
Range.opSlicemight be unsafe. This can't be@trusted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read this PR earlier and was confused by this as well. The opSlice being called could easily be exported by a range that I wrote, not even a range in Phobos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not always able to, often due to circular references. Phobos is full of circular templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding comments to the code with an explanation of why it's needed. If appropriate, the circumstances under which it could be removed. If it's a long explanation, add it to the PR and reference it from the code. Otherwise future readers of the code will be left wondering.
Progress to -dip1000 compatibility.