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 20937 - std.range.array of a lengthless range with indirection is not @safe #8202

Merged
merged 2 commits into from
Aug 31, 2021

Conversation

dukc
Copy link
Contributor

@dukc dukc commented Aug 19, 2021

Also replaced emplaceRef with emplace to remove dependency on DRuntime internals.

@dlang-bot
Copy link
Contributor

dlang-bot commented Aug 19, 2021

Thanks for your pull request and interest in making D better, @dukc! 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 coverage diff by visiting the details link of the codecov check)
  • 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
20937 normal std.range.array of a lengthless range with indirection is not @safe

Testing this PR locally

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

dub run digger -- build "stable + phobos#8202"

@thewilsonator
Copy link
Contributor

It seems this fails CI

@RazvanN7
Copy link
Collaborator

This seems to have uncovered an ICE in the compiler.

@RazvanN7
Copy link
Collaborator

Does this work as intended if emplaceRef is used?

@dukc
Copy link
Contributor Author

dukc commented Aug 20, 2021

Let's test...

@dukc
Copy link
Contributor Author

dukc commented Aug 20, 2021

Nope, does not. Changing back.

EDIT: oops, was watching old autotest results. Let's try once more

@dukc
Copy link
Contributor Author

dukc commented Aug 20, 2021

Now I'm definitely seeing an ICE with emplaceRef version. Hopefully this is the last switchback I'll do

@dukc
Copy link
Contributor Author

dukc commented Aug 20, 2021

Ouch, we have a heisenbug indeed. Even though I'm using the latest Linux dmd, compiling the same file with same flags as the autotester, the ICE does not reproduce locally.

EDIT: again bad reading of autotester reports on my part. I looked at last command before the error instead of the "command failed" line after the stack trace. Now I can reproduce.

@dukc
Copy link
Contributor Author

dukc commented Aug 20, 2021

DMD issue pinned - reported here: https://issues.dlang.org/show_bug.cgi?id=22228

@ibuclaw ibuclaw changed the title fix issue 20937 fix issue 20937 - std.range.array of a lengthless range with indirection is not @safe Aug 20, 2021
@dukc dukc force-pushed the fix20937 branch 2 times, most recently from 86cf356 to 512187c Compare August 23, 2021 20:08
@BorisCarvajal
Copy link
Member

Sorry, I should've target master in dlang/dmd#13007 . But you know, someone could say regressions go to stable, no exceptions.

I suppose an early merge should be possible. (HELP!)

@RazvanN7
Copy link
Collaborator

@dukc Please rebase against stable so that we can get this in.

@dukc dukc changed the base branch from master to stable August 24, 2021 20:19
@dukc
Copy link
Contributor Author

dukc commented Aug 24, 2021

Sorry, I should've target master in dlang/dmd#13007 . But you know, someone could say regressions go to stable, no exceptions.

I suppose an early merge should be possible. (HELP!)

Since the PR that caused the bug in first place was merged into stable, it was correct thing to do to target stable yourself. It's up to me to rebase in situations like these. Alternatively I could just have waited for the stable merge in DMD.

@dukc
Copy link
Contributor Author

dukc commented Aug 24, 2021

CircleCI fails because... DMD fails to build?

@RazvanN7
Copy link
Collaborator

Are you sure you are using the latest stable branch?

@dukc
Copy link
Contributor Author

dukc commented Aug 25, 2021

I update my local stable, then rebased my local PR branch against that and then force-pushed. And also I changed this PR:s target branch.

My fork repo still compares the PR branch against master though - how do I rebase that?

@RazvanN7 RazvanN7 closed this Aug 25, 2021
@RazvanN7 RazvanN7 reopened this Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants