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 13930, 19345 - Fix receiveOnly for non-assignable types #7661

Merged
merged 2 commits into from
Oct 15, 2020

Conversation

tom-tan
Copy link
Contributor

@tom-tan tom-tan commented Oct 13, 2020

This is a re-submit version of #7655.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @tom-tan! 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
13930 normal std.concurrency can't send immutable AA to another thread
19345 normal std.concurrency does not work with structs of const value type

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 + phobos#7661"

@thewilsonator
Copy link
Contributor

Looks all green this time, thanks!

@dlang-bot dlang-bot merged commit 6224cb0 into dlang:master Oct 15, 2020
@tom-tan tom-tan deleted the fix-issue13930 branch October 15, 2020 04:27
@kinke
Copy link
Contributor

kinke commented Oct 15, 2020

This might cause random segfaults: dlang/dmd#11868 (comment)

From a quick glance, the emplace usage seems invalid, as that's for uninitialized destination memory only.

@tom-tan
Copy link
Contributor Author

tom-tan commented Oct 17, 2020

This request uses emplace in two places. In the both cases, I used emplace as a workaround to initialize Variant with a struct with const members. Both cases internally need an assignment of the struct with const members

  • The first is in std.concurrency.receiveOnly to initialize the variable with the received value in the MessageBox.
    It internally initializes the value of Tuple!T ret after the variable declaration. I used emplace because ret cannot be initialized directly due to its variable scope.

    I have no idea for better solutions for it :-(

  • The second is in std.variant.Variant.opAssign called in Variant.this. It internally allocates the memory for the struct (in the case of large size of struct) using new and uses emplace to initialize its value.

    Using core.memory.malloc instead of new might be a better alternative.

Do you have any ideas or thoughts?

else
{
import core.lifetime : emplace;
emplace(p, rhs);
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, this requires the type UT to feature a constructor taking a T rhs lvalue argument, and should thus be equivalent to auto p = new UT(rhs). - I think that's not suited in general and should probably be replaced by a manual copy-emplace as used for the T.sizeof <= size case above. It cheats around immutability by faking a copy-construct, performing a bitcopy and invoking the postblit if necessary. By the looks of it, it currently seems to ignore copy ctors...

Copy link
Contributor

Choose a reason for hiding this comment

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

@pbackus
Copy link
Contributor

pbackus commented Feb 25, 2021

This PR introduced a regression:

https://issues.dlang.org/show_bug.cgi?id=21663

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