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

Implement the generation of multiple copy constructors for fields #16429

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RazvanN7
Copy link
Contributor

The scheme of copy constructor generation is quite simple:

  1. If a copy constructor is defined for any given struct => no copy constructor is generated
  2. If a struct does not define a copy constructor but has fields that define copy constructors => define a copy constructor for each unique combination of source-destination qualifiers that is encountered in the fields of the struct.
  3. If any of the generated copy constructors fail to typecheck, then they are annotated with disable; as such, copies that are possible are accepted, copies that are not will result in a compiler error.

@WalterBright wanted to implement a strategy where the most restrictive copy constructor is the one that is generated, however, I think that that behavior is very close to the current one where we define an unusable inout(inout) copy constructor. I think my approach is much simpler, since the compiler is let to decide whether the copies are possible or not, however it does probably come with a compile penalty for structs that define lots of copy constructor. However, I suspect that this situation is rare.

@dlang-bot
Copy link
Contributor

dlang-bot commented Apr 30, 2024

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

Auto-close Bugzilla Severity Description
20876 regression generated constructor always inout regardless of ability of fields to use inout

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

To target stable perform these two steps:

  1. Rebase your branch to upstream/stable:
git rebase --onto upstream/stable upstream/master
  1. Change the base branch of your PR to stable

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 + dmd#16429"

@RazvanN7
Copy link
Contributor Author

Why won't the bot auto-close the issue when this PR is merged? The commit message does contain "Fix Bugzilla Issue 20876"

@thewilsonator
Copy link
Contributor

maybe because the title doesn't start with that? I'm not sure what the exact regex used is

@RazvanN7
Copy link
Contributor Author

Thanks @thewilsonator , that was it.

@jmdavis
Copy link
Member

jmdavis commented Apr 30, 2024

Thanks. This seems like the right approach and is what I would have expected.

@RazvanN7
Copy link
Contributor Author

Ok, the headers need to be updated, but apart from that, it seems like the other failures are unrelated.


this(ref const Bar _) immutable
{
result ~= "C";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
result ~= "C";
result ~= "D";

@RazvanN7 RazvanN7 force-pushed the fix20876 branch 2 times, most recently from 728dae4 to 2be29c5 Compare May 3, 2024 08:18
@RazvanN7 RazvanN7 requested a review from ibuclaw as a code owner May 3, 2024 08:18
…onstructors for fields that define copy constructors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants