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

Add hasElaborateCopyConstructor trait #10265

Closed
wants to merge 4 commits into from

Conversation

edi33416
Copy link
Contributor

@edi33416 edi33416 commented Aug 2, 2019

This was requested in this druntime PR.

Exposing this behaviour straight from the compiler has two benefits:

  1. First, and most important imho, it avoids the risk of missing a corner case in the library implementation
  2. It significantly simplifies the library implementation

EDIT:

Please note that this trait returns false for types that have a postblit. Open question: should it return true?

After some further consideration I think that structures that define a postblit should also be considered to have an elaborate copy constructor. After all, this is what the library implementation do, so, imho, it would be confusing for the users for the trait to be different.

@dlang-bot
Copy link
Contributor

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

static assert(__traits(hasElaborateCopyConstructor, U!S));
static assert(__traits(hasElaborateCopyConstructor, SPostblit));

static assert(!__traits(hasElaborateCopyConstructor, NoCpCtor));
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a test with a nested struct too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is Outer.S. Am I missing something?

test/compilable/traits.d Show resolved Hide resolved
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.

This implementation will return true when __traits(hasElaborateCopyConstructor) is used with a type that is not a struct. You should probably add some tests to verify that other types like classes or basic types return false for this check.

@WalterBright
Copy link
Member

There's some confusion over what an "elaborate" copy constructor even is.

https://dlang.org/phobos/std_traits.html#hasElaborateCopyConstructor

That says elaborate means having a postblit. But you say "I think that structures that define a postblit should also be considered to have an elaborate copy constructor" meaning this PR defines it as something else?

Putting a trait in the core language means the core language must have a proper definition of it. C++ has no notion of an "elaborate" copy constructor. I confess I have no idea what it should mean and what such a definition would be good for.

If "elaborate copy constructor" is a euphemism for "postblit", why not call it "postblit"?

I remember back when we were trying to come up with a name for immutable types. People came up with readonly, invariant, etc. When people would ask what readonly or invariant meant, we'd respond with "it means immutable". Finally we got thwacked with a clue-by-four and noticed the obvious - name it "immutable". Didn't have to explain it after that.

@TurkeyMan
Copy link
Contributor

There is postblit, and there is copy constructor. I think the interesting fact is "do I need to run special code to copy this".
If the result is false, then memcpy is a valid copy. If it is true, it must execute a function.

The reason this should be a trait is because as we've shown, the compiler will use internal logic to determine if it should call a postblit or copy constructor, and it's very hard to reproduce that logic in library. It is critical that library can sample the same condition, otherwise copying mechanics will have strange edge cases in library code.

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Aug 3, 2019

So, by my reasoning 'elaborate copy' means "something other than a straight binary copy".

I would apply the same terminology to 'elaborate move', 'elaborate construction' (something other than a blit of init), etc.

src/dmd/traits.d Outdated
}
else if (e.ident == Id.hasElaborateCopyConstructor)
{
return sd.hasCopyCtor || sd.postblit !is null ? True() : False();
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, compare this to the library solution ;) ... this builds confidence.

@WalterBright
Copy link
Member

The whole naming and documentation issue can be avoided by simply having two traits:

  1. hasCopyConstructor
  2. hasPostblit

and voila! No confusion, no extra explanation needed, and it's more flexible - the user can decide if he's checking for postblit too or not.

@TurkeyMan
Copy link
Contributor

That's true, but then we also have to be certain that the compiler internally performs exactly hasCopyConstructor || hasPostblit when deciding to do something 'elaborate'... The traits need to reflect exactly the compilers internal logic, whatever it is.

@TurkeyMan
Copy link
Contributor

Remember this may get complicated with concepts like copy ctor overloads, move construction (if it turns out that way), etc...

@edi33416
Copy link
Contributor Author

edi33416 commented Aug 5, 2019

Please also check out the updated library PR

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Aug 5, 2019

Please also check out the updated library PR

I just checked, and it looks like the old PR... did push?
Shouldn't it be with this __traits PR just this:

enum hasElaborateCopyConstructor(T) = __traits(hasElaborateCopyConstructor, T);

??

@edi33416
Copy link
Contributor Author

edi33416 commented Aug 6, 2019

The druntime PR provides an alternative to the issue without requiring the __traits, at the suggestion of @andralex . I apologize for being unclear.

@TurkeyMan
Copy link
Contributor

Why has this stalled?

@edi33416
Copy link
Contributor Author

edi33416 commented Aug 19, 2019

@WalterBright @andralex @RazvanN7 can this be merged? This would allow the corresponding druntime PR to move forward.
Should this target stable?

@12345swordy
Copy link
Contributor

@thewilsonator can you help with this?

src/dmd/traits.d Outdated Show resolved Hide resolved
@edi33416
Copy link
Contributor Author

edi33416 commented Sep 2, 2019

If this same logic repeated somewhere in the assignment semantic code when the compiler determines how to handle copying?
It would be good to factor the logic in that location into a function, and call that same function from here. That way we can't fall out of sync again in the future.

I agree. Refactored as @TurkeyMan suggested.

@WalterBright @RazvanN7 what do you think?

@TurkeyMan
Copy link
Contributor

It would be cool to unblock dlang/druntime#2780...

@12345swordy
Copy link
Contributor

ping the @thewilsonator

@n8sh
Copy link
Member

n8sh commented Sep 6, 2019

That's true, but then we also have to be certain that the compiler internally performs exactly hasCopyConstructor || hasPostblit when deciding to do something 'elaborate'... The traits need to reflect exactly the compilers internal logic, whatever it is.

Since the compiler already defines & uses fields named postblit and hasCopyCtor but doesn't already have anything internal called hasElaborateCopyCtor, I think "the traits need to reflect exactly the compilers internal logic, whatever it is" is a point towards exposing the existing fields as traits per #10265 (comment).

@TurkeyMan
Copy link
Contributor

I had that thought too, and I agree as long as we can be confident that the proper logic is and will always be the disjunction of those 2 values... so, can we be sure of that? is hasPostblit || hasCopyCtor absolutely solid?

@andralex
Copy link
Member

andralex commented Sep 6, 2019

How about the converse - we define hasTrivialCopyConstructor? Clear, unambiguous, nice name.

@TurkeyMan
Copy link
Contributor

As a traits? That's this expression: !hasPostblit && !hasCopyCtor, which will find the same argument for making them 2 independent checks equally apply.

@12345swordy
Copy link
Contributor

I can't add tags such as "blocked" as it quite clearly this is blocking certain c++ std libraries from integrating in the d druntime.

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Oct 6, 2019

This is still dangling :(
Someone make a choice and merge something, I don't care what form it takes.

@WalterBright
Copy link
Member

This is still dangling

hasCopyConstructor, hasPostblit

As defined in this PR, the two are combined into a new, and pointless, jargon. The next PR will be to split it into two. Just do it as two to begin with, then it doesn't need explaining.

@TurkeyMan
Copy link
Contributor

Or the thing Andrei says...?
Either way.

@adamdruppe
Copy link
Contributor

i forked and revived to split the traits #10528

@PetarKirov
Copy link
Member

Closing in favor of #10528.

@PetarKirov PetarKirov closed this Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.