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.

Sign up for GitHub

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

Jump to bottom

Implement copy constructor #8688

Merged

andralex merged 31 commits into dlang:master from RazvanN7:CpCtor

Mar 28, 2019

Contributor

RazvanN7 commented Sep 11, 2018 •

edited

As per DIP: [1].

More tests will be added soon.

[1] dlang/DIPs#129

Contributor

dlang-bot commented Sep 11, 2018

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 annotated coverage diff directly on GitHub with CodeCov's browser extension
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 + dmd#8688"

RazvanN7 requested review from ibuclaw and dnadlinger as code owners

September 11, 2018 15:04

Contributor

jacob-carlborg commented Sep 11, 2018

Making the @implicit keyword a compiler recognizes UDA would avoid some breaking changes.

UplinkCoder reviewed

View reviewed changes

src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

RazvanN7 commented

View reviewed changes

src/dmd/dstruct.d Outdated Show resolved Hide resolved

WalterBright reviewed

View reviewed changes

src/dmd/expression.d Outdated Show resolved Hide resolved

WalterBright reviewed

View reviewed changes

src/dmd/expression.d Show resolved Hide resolved

WalterBright reviewed

View reviewed changes

src/dmd/expressionsem.d Outdated Show resolved Hide resolved

WalterBright reviewed

View reviewed changes

src/dmd/semantic3.d Outdated Show resolved Hide resolved

WalterBright reviewed

View reviewed changes

src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

WalterBright reviewed

View reviewed changes

src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

WalterBright reviewed

View reviewed changes

src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

WalterBright reviewed

View reviewed changes

src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

WalterBright reviewed

View reviewed changes

src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

WalterBright reviewed

View reviewed changes

src/dmd/expression.d Show resolved Hide resolved

WalterBright reviewed

View reviewed changes

src/dmd/expression.d Outdated Show resolved Hide resolved

WalterBright reviewed

View reviewed changes

src/dmd/expressionsem.d Outdated Show resolved Hide resolved

WalterBright reviewed

View reviewed changes

src/dmd/expressionsem.d Outdated Show resolved Hide resolved

WalterBright reviewed

View reviewed changes

src/dmd/expressionsem.d

+                                              if (!einit)
+                                                  return setError();
+                                              Expression e;

Member

WalterBright Sep 13, 2018

Insert a comment showing what expression is being built.

Contributor Author

RazvanN7 Sep 13, 2018

Isn't the one on line 7742 enough?

WalterBright reviewed

View reviewed changes

src/dmd/expressionsem.d

+                                          {
+                                              if (!e2x.type.implicitConvTo(e1x.type))
+                                              {
+                                                  exp.error("conversion error from `%s` to `%s`",

Member

WalterBright Sep 13, 2018

I imagine this error message would be completely baffling to a user.

Contributor Author

RazvanN7 Sep 13, 2018

It was there before. It has nothing to do with this PR

WalterBright reviewed

View reviewed changes

src/dmd/func.d Outdated Show resolved Hide resolved

RazvanN7 force-pushed the CpCtor branch from cdd8100 to a58aefe Compare

September 24, 2018 13:08

RazvanN7 force-pushed the CpCtor branch from 2fb327a to a97bf13 Compare

October 8, 2018 09:21

RazvanN7 force-pushed the CpCtor branch 6 times, most recently from 9b40369 to 59bf730 Compare

October 22, 2018 14:02

RazvanN7 added 17 commits

March 27, 2019 10:58


          Fix function overload resolution

2e97c40


          Postblit precedence

908b1e9


          Fix error message for argument with copy constructor

1214ce6


          Add copy ctor generation without implicit

ec0d6f9


          Delete wrongfully added code from rebase

b85d853


          Remove last traces of implicit + update .h files

912c304


          Add scope parameter to all callMatch calls in dmd/dtemplate.d

0bdd807


          Delete generation of opAssign and tweak generation of field copy cons…

95f2e59

…tructor


          Add possiblity of defining default parameters for copy constructors

69b1cea


          Modify generation of copy constructors to match the DIP

a9e4c1d


          solve mir failure

7c07c3f


          Address feedback

30ffb88


          Delete isCopyCtor method

af93051


          Make definition of both rvalue constructor and copy constructor an error

db88573


          Add changelog entry


          Add return scope for generated copy constructor parameter

95777c2


          Address feedback

1aeffd1

RazvanN7 force-pushed the CpCtor branch from bf23af4 to ef3ce4b Compare

March 27, 2019 12:10

Contributor Author

RazvanN7 commented Mar 27, 2019

@jacob-carlborg I fixed the changelog and the inline comments.

@wilzbach The only new thing added here is that the compiler inserts calls to the copy constructor where it used to insert for the postblit. This is tested. Moreover, there is a reference counted array implementation that uses the copy constructor and passes all the tests locally [1].

[1] dlang/druntime#2348


          Fix changelog + add tests

17b174c

RazvanN7 force-pushed the CpCtor branch from ef3ce4b to 17b174c Compare

March 27, 2019 12:58

RazvanN7 removed Needs Changelog Pending DIP Approval labels

Contributor

jacob-carlborg commented Mar 27, 2019

Changelog looks good, almost a bit too long.

andralex dismissed stale reviews from jacob-carlborg and wilzbach

March 28, 2019 01:18

Changelog has been beefed up.

Member

andralex commented Mar 28, 2019

Failures seem unrelated, I'll pull the plug on this.

andralex merged commit 588e8fd into dlang:master

Contributor

MoonlightSentinel commented Apr 8, 2022

This introduced a regression: https://issues.dlang.org/show_bug.cgi?id=22997
git bisect blames commit db88573

RazvanN7 mentioned this pull request

Do not include rvalue constructor in Algebraic if it already has a copy constructor libmir/mir-core#81

Merged

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

Reviewers

andralex andralex left review comments

UplinkCoder UplinkCoder left review comments

PetarKirov PetarKirov left review comments

atilaneves atilaneves left review comments

thewilsonator thewilsonator left review comments

iain-buclaw-sociomantic iain-buclaw-sociomantic left review comments

stefan-koch-sociomantic stefan-koch-sociomantic left review comments

WalterBright WalterBright approved these changes

jacob-carlborg jacob-carlborg left review comments

wilzbach wilzbach left review comments

ibuclaw Awaiting requested review from ibuclaw ibuclaw is a code owner

dnadlinger Awaiting requested review from dnadlinger