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

Comments

Translate _d_array{,set}ctor to templates#2655

Merged
dlang-bot merged 3 commits intodlang:masterfrom
Vild:ConvertArrayCtorHook
Jul 10, 2019
Merged

Translate _d_array{,set}ctor to templates#2655
dlang-bot merged 3 commits intodlang:masterfrom
Vild:ConvertArrayCtorHook

Conversation

@Vild
Copy link
Contributor

@Vild Vild commented Jun 27, 2019

DMD PR: dlang/dmd#10102

  • This adds rt.util.array to mak/COPY as its a dependency for _d_arrayctor
  • _d_array{,set}ctor uses @trusted to not break code, pure and nothrow can be automatically deduced by the compiler
  • _d_arraysetctor no longer returns a return value as it causes warnings:
std/container/array.d(97): Warning: calling rt.array.constructor._d_arraysetctor!(const(Array!int))._d_arraysetctor without side effects discards return value of type const(Array!int)[], prepend a cast(void) if intentional

@dlang-bot
Copy link
Contributor

dlang-bot commented Jun 27, 2019

Thanks for your pull request and interest in making D better, @Vild! 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 fetch digger
dub run digger -- build "master + druntime#2655"

@Vild Vild force-pushed the ConvertArrayCtorHook branch from 2a15fba to 5cb42f6 Compare June 28, 2019 15:15
@Vild Vild force-pushed the ConvertArrayCtorHook branch from 5cb42f6 to 774f81a Compare June 29, 2019 22:25
}
}
catch (Exception o)
{
Copy link
Contributor

@jpf91 jpf91 Jun 30, 2019

Choose a reason for hiding this comment

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

Just for reference, this means calling destructors will not be done if the postblit threw a non-exception throwable. IIRC the language spec allows this behaviour @WalterBright @andralex ?

Copy link
Contributor

@JinShil JinShil Jun 30, 2019

Choose a reason for hiding this comment

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

See

try
{
for (i = 0; i < to.length; i++)
{
// Copy construction is defined as bit copy followed by postblit.
memcpy(to.ptr + i * element_size, from.ptr + i * element_size, element_size);
ti.postblit(to.ptr + i * element_size);
}
}
catch (Throwable o)
{
/* Destroy, in reverse order, what we've constructed so far
*/
while (i--)
{
ti.destroy(to.ptr + i * element_size);
}
throw o;
}
for the genesis of that code.

@jpf91
Copy link
Contributor

jpf91 commented Jun 30, 2019

LGMT, but the windows Makefile needs to be fixed. @Vild is this PR ready?

{
walker--;
auto elem = cast(Unqual!T*)&p[walker];
static if (__traits(hasMember, T, "__xdtor") &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this code be calling object.destroy instead?

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 changed it object.destroy and object._postblitRecurse for the postblit.

@Vild Vild force-pushed the ConvertArrayCtorHook branch from 774f81a to c5fffe3 Compare June 30, 2019 20:07
@Vild
Copy link
Contributor Author

Vild commented Jun 30, 2019

I would call this PR ready.

About the windows build problem, I'm not sure why this is happening.

copy src\rt\util\array.d  import\rt\util\array.d
The system cannot find the path specified.
        0 file(s) copied.

But the file exist when I check: ls src/rt/util/array.d

@JinShil
Copy link
Contributor

JinShil commented Jun 30, 2019

@rainers We're getting the following build error on Windows

..\..\druntime\import\rt\array\construction.d(32): Error: module `array` is in file 'rt\util\array.d' which cannot be read

It appears all the right makefile modifications have been made. Do you happen to know why this error is occurring?

@rainers
Copy link
Member

rainers commented Jul 1, 2019

About the windows build problem, I'm not sure why this is happening.

copy src\rt\util\array.d  import\rt\util\array.d
The system cannot find the path specified.

The problem is that the output directory does not exist. It should be added similar to https://github.com/dlang/druntime/blob/master/mak/WINDOWS#L57

While you are at it, you could also add core\sys\netbsd\sys and core\sys\netbsd.

@Vild Vild force-pushed the ConvertArrayCtorHook branch from c5fffe3 to 7e9b792 Compare July 1, 2019 17:25
@thewilsonator
Copy link
Contributor

It seems like the exception paths are not tested, could you add some tests that exercise those? Otherwise looks good.

Vild added 2 commits July 8, 2019 14:24
Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>
Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>
@Vild Vild force-pushed the ConvertArrayCtorHook branch 2 times, most recently from bd31a2f to fb70963 Compare July 9, 2019 13:41
Signed-off-by: Dan Printzell <xwildn00bx@gmail.com>
@Vild Vild force-pushed the ConvertArrayCtorHook branch from fb70963 to b2b03a0 Compare July 9, 2019 17:31
@Vild
Copy link
Contributor Author

Vild commented Jul 9, 2019

@thewilsonator done

This PR is ready to be merged.

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