Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Comments

Rewrite object.dup to work with scope and copy constructors#3474

Merged
dlang-bot merged 3 commits intodlang:masterfrom
dkorpel:dup-scope
May 28, 2021
Merged

Rewrite object.dup to work with scope and copy constructors#3474
dlang-bot merged 3 commits intodlang:masterfrom
dkorpel:dup-scope

Conversation

@dkorpel
Copy link
Contributor

@dkorpel dkorpel commented May 17, 2021

.dup and .idup used to be built-in properties of arrays along with .sort and .reverse.
In 2014 they got implemented as druntime functions, but being 7 years old, they now have the following shortcomings:

This PR addresses the first two points, the third point depends on a dmd change.

The copy-constructor test are taken from #3139.
The post-blit / copy-constructor logic uses copyEmplace.

@dkorpel dkorpel changed the title Rewrite object.dup to work with scope and copy-constructors Rewrite object.dup to work with scope and copy constructors May 17, 2021
@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#3474"

static assert( __traits(compiles, () { [].idup!ISthrow; }));
static assert(!__traits(compiles, () nothrow { [].idup!ISthrow; }));
static assert( __traits(compiles, () { [].idup!ISunsafe; }));
static assert(!__traits(compiles, () @safe { [].idup!ISunsafe; }));
Copy link
Contributor

@MoonlightSentinel MoonlightSentinel May 17, 2021

Choose a reason for hiding this comment

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

Remove the useless static assert( __traits(compiles, ...) and move them into a if (false) { ... } if they shouldn't be executed. That ensures proper error messages when the instantiation fails due to some other error.

Also move the instantiation out of the static assert(!__traits(compiles, ...) and only check that the instance cannot be called in a @safe function. Otherwise the test can pass due to other failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I now just call the valid cases directly and only use the trait to test the function attributes.
If they don't compile because of other reasons, the valid cases should fail first with a decent error message.

@dkorpel
Copy link
Contributor Author

dkorpel commented May 17, 2021

By the way, I noticed that dup seems to be a CTFE builtin, while idup isn't? I'm not sure if the code in the __ctfe branch is actually used.

@dkorpel
Copy link
Contributor Author

dkorpel commented May 21, 2021

I don't know what's happening with the auto-tester... One moment it's reporting "Pending: 8", another moment it's all green.

@aG0aep6G
Copy link
Contributor

I don't know what's happening with the auto-tester... One moment it's reporting "Pending: 8", another moment it's all green.

"Pending: 8" is correct. The "Details" page is all green, but those results are old. Tests that haven't even started yet are not visible there.

You can also find your PR here: https://auto-tester.puremagic.com/pulls.ghtml?projectid=1. There you see that only two results are up to date (bright green). The other eight are not up to date but they passed previously (muted green).

Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for taking care of this.

@RazvanN7 RazvanN7 added the 72h no objection -> merge The PR will be merged if there are no objections raised. label May 25, 2021
@RazvanN7 RazvanN7 added auto-merge and removed 72h no objection -> merge The PR will be merged if there are no objections raised. labels May 28, 2021
@dlang-bot dlang-bot merged commit d72a0d0 into dlang:master May 28, 2021
} ();

static if (__traits(hasPostblit, T))
_doPostblit(res);
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is problematic because the remaining elements are not initialized to T.init if the postblit/copy constructor throws. The GC will still destroy the entire element.

https://issues.dlang.org/show_bug.cgi?id=21983
Attempted fix: #3483

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants