Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Remove string mixin from _d_arrayappendcTX #3870

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

dkorpel
Copy link
Contributor

@dkorpel dkorpel commented Jul 4, 2022

It was introduced in #2632, but the two branches look identical to me.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @dkorpel! 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

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.

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 "master + druntime#3870"

private enum _d_arrayappendcTXBody = q{
ref Tarr _d_arrayappendcTX(return ref scope Tarr px, size_t n) @trusted pure nothrow
{
pragma(inline, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

while we're changing this, please move this pragma outside of the function

Copy link
Member

Choose a reason for hiding this comment

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

Why have it at all

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

otherwise looks good

@RazvanN7
Copy link
Contributor

RazvanN7 commented Jul 5, 2022

cc @teodutu . Does this affect your current work in any way?

@teodutu
Copy link
Member

teodutu commented Jul 5, 2022

@RazvanN7, no. nothrow was added because at that time it wasn't inferred properly, but now it seems to be fine.

@dkorpel dkorpel marked this pull request as ready for review July 5, 2022 15:55
@RazvanN7
Copy link
Contributor

RazvanN7 commented Jul 6, 2022

Maybe a rebase will fix the vibe.d failure?

@RazvanN7 RazvanN7 merged commit d44e4e1 into dlang:master Jul 8, 2022
@RazvanN7
Copy link
Contributor

RazvanN7 commented Jul 8, 2022

Vibe failure is unrelated, so merging this as is.

@dkorpel dkorpel deleted the remove-mixin branch July 8, 2022 10:00
@dkorpel
Copy link
Contributor Author

dkorpel commented Jul 8, 2022

It looks related to me:

core.exception.AssertError@src/dmd/dinterpret.d(4878): CTFE cannot interpret _d_arrayappendcTX!

I think the pragma(inline, false); was actually needed.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Jul 8, 2022

Hmm, I couldn't see that error message. Maybe it's required that the function body is a string so that ctfe can interpret it?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants