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

Comments

Fix dup and idup property in the presence of copy constructors#3139

Closed
RazvanN7 wants to merge 1 commit intodlang:masterfrom
RazvanN7:Dup_CC
Closed

Fix dup and idup property in the presence of copy constructors#3139
RazvanN7 wants to merge 1 commit intodlang:masterfrom
RazvanN7:Dup_CC

Conversation

@RazvanN7
Copy link
Contributor

@RazvanN7 RazvanN7 commented Jun 17, 2020

This is a first step to fixing: https://issues.dlang.org/show_bug.cgi?id=20879 . There are a ton of other places that need fixing, but I want to tackle these in small incremental changes.

@dlang-bot
Copy link
Contributor

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

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

Choose a reason for hiding this comment

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

Shouldn't the tests also test that the code actually works, i.e. the copy constructor of all array elements gets called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added one test

Copy link
Contributor

Choose a reason for hiding this comment

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

Also missing implementation, and a test for a struct that contains pointers and has a this(ref const typeof(this)) copy ctor. Current dup/idup won't compile with those (they should). __traits(hasCopyConstructor, T) is really not sufficient.

{
foreach (i, ref elem; src)
{
dst[i].__ctor(elem);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if it throws? Or rather, what if it throws, exception is caught, and later GC collects the array? Will it call any dtors? All of them? On a partially-initialized array?

Copy link
Member

Choose a reason for hiding this comment

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

Yah, this needs care:

size_t i = 0;
scope (failure)
{
    for (size_t j = 0; j < i; ++j)
        dst[j].__dtor();
    delete dst;
}
for (; i < src.length; ++i)
    dst[i].__ctor(elem);

Copy link
Member

Choose a reason for hiding this comment

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

Well not delete :)

Copy link
Contributor

@radcapricorn radcapricorn Jun 18, 2020

Choose a reason for hiding this comment

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

__xdtor. If exists. And probably, before this call, not a _d_newarrayU, but a _d_newarrayiT. ;)
...or emplace remaining initializers in case of failure.

...on third thought, calling dtors here is probably unnecessary. Just need to make sure the remainder is in .init state. Although, what of the failed element itself? Are we allowed to overwrite it with .init? I mean, the GC better call those dtors if it collects. Or do defensively call dtors and put .init? Bah...

Copy link
Contributor

Choose a reason for hiding this comment

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

Direct call to __ctor doesn't seem to write .init. Bad things may happen (c).

Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably call _d_arrayctor. Otherwise, we'll need to replicate the logic there as well. It seems that _d_arrayctor already implements the logic necessary to handle exceptions.

Also, _d_arrayctor uses postblitRecurse which knows about the new copyCtor.

There is, however, an issue with _d_arrayctor: it does a memcpy before calling postblitRecurse, which defies the purpose of the copyCtor, but this is an easy fix (I'll open a PR for it)

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.

6 participants