Move `emplace` to druntime for symmetry with `destroy`. #2398

TurkeyMan · 2018-12-07T08:12:06Z

destroy() is in object.d, which makes sense, because one of the most primitive operations you can perform is to cause destruction of an object.
Strangely, we can't construct objects though... because emplace() is in phobos, which isn't helpful for projects that don't link phobos. (it's huge, and it's not nothrow @nogc)

I think emplace() really needs to live right next to destroy(); it's a direct counterpart, so here it is.
If this is accepted, we'll make phobos emplace alias this one moved here.

dlang-bot · 2018-12-07T08:12:07Z

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

jacob-carlborg · 2018-12-07T08:44:15Z

I don't think this is a good idea. There's already way too much stuff in object.d.

JinShil · 2018-12-07T08:59:51Z

There's already way too much stuff in object.d.

I tried to do something about that in #2222, but then I ran into https://issues.dlang.org/show_bug.cgi?id=19014 Maybe that can move forward now since time has passed.

TurkeyMan · 2018-12-07T09:02:49Z

There's already way too much stuff in object.d.

It's just one function, and it's a direct missing complement to one of the most important functions in object.d.

By your argument, destroy needs to move somewhere else... but either way, emplace needs to go there too. It's not okay that emplace and destroy aren't together.

emplace is harder to implement than destroy, and no user should ever attempt to get it right themselves. It's not reasonable that it be in phobos, it's too low-level and foundational.

TurkeyMan · 2018-12-07T09:05:25Z

@JinShil If you wanna move destroy, then please either merge this first, then move them both, or move emplace at the same time...

JinShil · 2018-12-07T09:14:51Z

I'm not going to do anything. I just wanted to point out that there is a way forward to reduce the size of object.d. I have no opinion on whether this should be merged or not, and I am no longer working on the team.

TurkeyMan · 2018-12-07T09:19:06Z

Okay. Well while I sympathise that object.d is kinda big, and it could be smaller, I find that issue microscopic compared to the fact that emplace isn't together with destroy, which is catastrophic for anyone that doesn't link phobos... you can't build anything meaningful without construction. You can't write any container object without construction. I can't move on std.vector or any of those without construction, etc. I've been annoyed about this for years... just gotta fix it.

schveiguy · 2018-12-07T16:18:54Z

Would it be a reasonable compromise to put emplace into its own module in core?

The truth is, D supplies 2 forms of construction -- one on the stack, and one on the heap. The reason destroy is in druntime is because it's the complement to heap construction for deterministic destruction. It's needed to replace delete, which is deprecated.

I agree it would be nice to have emplace in druntime, that's really where it belongs. But not sure if it needs to be in object.d There's lots of code that might use destroy but not emplace, and emplace is way more hairy, and pulls in more things. Having it in core means you don't need to import phobos, but that you don't have to import it for every module.

TurkeyMan · 2018-12-07T19:10:17Z

I have no issue with it being in another module, but I do think it should be in the same place as destroy...
If someone wants to move destroy somewhere else, I'll put emplace there.

TurkeyMan · 2018-12-07T19:23:31Z

D supplies 2 forms of construction

I'd argue that the language supplies no forms of construction, only allocation (with implicit construction). The language has no construction syntax (ie, C++ placement new), and so we write it awkwardly in a library (emplace), which is chaos to implement!

You can't write a whole lot of tooling and container objects without being able to construct.

schveiguy · 2018-12-07T20:07:11Z

If someone wants to move destroy somewhere else

destroy is not coupled with emplace. You can destroy objects that were not created with emplace. Why does it need to be in the same module?

In any case, destroy can't be moved, as then literally all code that uses destroy would break.

TurkeyMan · 2018-12-07T23:17:28Z

Which is exactly why I put emplace here ;)

JinShil · 2018-12-08T00:12:41Z

In any case, destroy can't be moved, as then literally all code that uses destroy would break.

I believe destroy and other lifetime related functions can be moved to another module that can then be publicly imported by object.d. That, I think, will help clean up the object.d mess and hopefully make this PR more palatable. See #2222 for a first attempt.

schveiguy · 2018-12-08T02:25:08Z

moving destroy to another module, and then publicly importing that module, is not much different than having it here -- every module will import it. What I meant was that I didn't necessarily want to import emplace at all in all modules.

In other words, it would be perfectly acceptable to have emplace live in hypothetical core.emplace module, and leave destroy in object.d. This way, emplace isn't imported and parsed for all modules, regardless of whether they use emplace or not. The problem we're solving here is

Strangely, we can't construct objects though... because emplace() is in phobos

If we were also adding destroy, it would be possible to also have it live in a separate optional module, but not now.

JinShil · 2018-12-08T04:09:34Z

moving destroy to another module, and then publicly importing that module, is not much different than having it here -- every module will import it.

Understood, but that's not the motivation for moving it. object.d has too much stuff in it which causes problems when creating pull requests against it; see the diff for #2194. All that PR did was moving some implementations behind a version block and the diff algorithm marked the entire file with changes. It's unmaintainable. @jacob-carlborg's concern is right that this PR only makes matters worse, and that, unfortunately, ~~subtracts~~ distracts from the merit of this PR. Another attempt at #2222 would probably significantly help this PR.

TurkeyMan · 2018-12-08T21:09:25Z

The thing that's bullshit about this whole community is that people will readily complain and bikeshed when someone tries to get something done, but they wouldn't do it themselves.
Strong technical rejection is always proper, but trivial bikeshedding should be banned by anyone who isn't prepared to make their proposed amendments themselves.

This is holding me up. Let me fix it, or fix it yourself... but don't be a barrier to both.

JinShil · 2018-12-08T23:06:14Z

In my attempt with #2222 I had to fix 2 other bugs: dlang/dmd#8519 and dlang/dmd#8519 and eventually got stalled on https://issues.dlang.org/show_bug.cgi?id=19014. I didn't get any shortcuts; I did what needed to be done and even made time to help you, @TurkeyMan , with a bug I couldn't care less about: dlang/dmd#8631 and a number of other bugs for other people.

When working on D (or any other software for that matter) this is how it often goes: https://www.youtube.com/watch?v=AbSehcT19u0

The size of object.d is a legitimate technical concern, yet I already admitted that it is a distraction from the merits of this PR. This PR could be merged regardless (I defer to others' judgment on that), but someone, someday will eventually need to deal the object.d monstrosity and ignoring it now (actually adding to it) is just passing the buck.

TurkeyMan · 2018-12-09T06:29:07Z

I appreciate you working through those issues.
I just get frustrated with wasting time I don't have to begin with. Every time I try and do anything that should take an hour, it invariably takes a week (or more).
I don't think any anyone could argue that 'malloc' and 'free' belong together, 'new' and 'delete', etc. They shouldn't be separated, that wouldn't make sense to anyone.

Is destroy definitely going to move? I can put emplace where it will move to, but it needs to move if so... They should be kept together, and if it's not actually gonna move, then one more function next to destroy won't change the existing undesirable situation in any significant way.

JinShil · 2018-12-09T06:42:20Z

Is destroy definitely going to move?

I don't know. Let's wait to see what happens with the PR. Perhaps my work and our concerns were all for naught. There are competing opinions here, and I can't predict what wins. I have no opinion about whether emplace and destroy belong together. I just want to remove the distraction of object.d so we can have that discussion.

jacob-carlborg · 2018-12-09T20:01:47Z

I don't think any anyone could argue that 'malloc' and 'free' belong together, 'new' and 'delete', etc. They shouldn't be separated, that wouldn't make sense to anyone.

One could argue that the opposite of destroy is new, i.e. the keyword.

schveiguy · 2018-12-09T20:04:22Z

people will readily complain and bikeshed

This is NOT bikeshedding. Once you add a symbol to object.d, it has to stay there forever (or at least always be imported automatically). If you put it in a different module inside druntime, then it's opt-in to import. This is a huge difference.

Think of it this way, if you import core.emplace, you will get emplace and destroy. Why is that a problem?

If it's determined that it would be better suited in object.d, we can move it there later. If we put it in object.d now, and decide later it shouldn't be there, the decision cannot be reversed.

I've said my piece. I'm not the final decision maker anyway, but it's frustrating not to be able to bring up technical problems with a PR without getting yelled at.

TurkeyMan · 2018-12-09T21:09:49Z

One could argue that the opposite of destroy is new, i.e. the keyword.

You could argue, but I think you'd fail.
new allocates memory, destroy performs destruction, which is the inverse of performing construction. Neither emplace or destroy have anything to do with memory allocation, and that's the whole point.

src/object.d

wilzbach · 2018-12-18T15:24:16Z

Think of it this way, if you import core.emplace, you will get emplace and destroy. Why is that a problem?

Can we just move it in core somewhere, s.t. it can be used in druntime and postpone the discussion on whether it should be in object.d?

PetarKirov · 2018-12-19T07:19:36Z

Yes, please! Let's use either core.lifetime or core.internal.lifetime.

src/core/internal/traits.d

thewilsonator · 2018-12-31T07:16:43Z

Moved to core/lifetime.d.

changelog/emplace.dd

jacob-carlborg

I don't see any tests for C++ and Objective-C classes/interfaces
Are there any tests for CTFE?

jacob-carlborg · 2018-12-31T11:22:24Z

src/core/lifetime.d

+address. If `T` is a class, initializes the class reference to null.
+Returns: A pointer to the newly constructed object (which is the same
+as `chunk`).
+ */


Missing Params section

Please add a blank line between the sections

jacob-carlborg · 2018-12-31T11:23:18Z

src/core/lifetime.d

+Returns: A pointer to the newly constructed object (which is the same
+as `chunk`).
+ */
+T* emplace(T, Args...)(T* chunk, auto ref Args args)


Missing Params section

Please add a blank line between the sections

jacob-carlborg · 2018-12-31T11:23:34Z

src/core/lifetime.d

+This function is `@safe` if the corresponding constructor of `T` is `@safe`.
+Returns: The newly constructed object.
+ */
+T emplace(T, Args...)(T chunk, auto ref Args args)


Missing Params section

Please add a blank line between the sections

jacob-carlborg · 2018-12-31T11:24:33Z

src/core/lifetime.d

+///
+@safe unittest
+{
+    () @safe {


Why is this necessary when the whole unit test block is @safe?

jacob-carlborg · 2018-12-31T11:25:13Z

src/core/lifetime.d

+This function can be `@trusted` if the corresponding constructor of `T` is `@safe`.
+Returns: The newly constructed object.
+ */
+T emplace(T, Args...)(void[] chunk, auto ref Args args)


Missing Params section

Please add a blank line between the sections

jacob-carlborg · 2018-12-31T11:25:38Z

src/core/lifetime.d

+`T` is `@safe`.
+Returns: A pointer to the newly constructed object.
+ */
+T* emplace(T, Args...)(void[] chunk, auto ref Args args)


Missing Params section

Please add a blank line between the sections

jacob-carlborg · 2018-12-31T11:28:17Z

src/core/lifetime.d

+    I i = void;
+    emplace(&i);
+    assert(i is null);
+}


Should there be a test that verifies that the constructor has been called?

TurkeyMan · 2019-01-01T21:52:39Z

@thewilsonator Just an FYI, I think it's considered extremely poor form to rebrand someone's git commit metadata when you amend. I've seen this occur on a few other PR's, I'd suggest that when you amend someone's PR's, take extra care not to erase the original authors attribution and commit metadata.

TurkeyMan · 2019-01-03T09:01:47Z

@jacob-carlborg I don't want to change anything about the implementation in this PR. It's a perfect clone of phobos to the extent we know it works. If we tidy the things you say, I'd like to see that in a separate PR where it's not conflated with moving it to druntime.

jacob-carlborg · 2019-01-03T09:47:52Z

I don't want to change anything about the implementation in this PR. It's a perfect clone of phobos to the extent we know it works

Fine.

WalterBright · 2019-01-03T10:56:19Z

src/core/internal/traits.d As far as I can tell, all the changes in this file are reformatting. Please put reformatting in a separate file, as this diff is way way too large to start adding irrelevant changes.

TurkeyMan · 2019-01-03T18:51:57Z

I see what's happened, this PR was actually the original patch to introduce those traits, but nick extracted that code into a separate PR and merged, but reformatted it in the process. When I rebased, my (original) formatting changes were the only differences that remained.
I moved those traits over from phobos verbatim, the formatting in this patch is identical to the phobos formatting. I'm actually a bit upset that it was disturbed when it was merged separately.

TurkeyMan · 2019-01-03T18:52:33Z

The idea here was to have ZERO delta from the phobos code, and that's what's in this patch ;)

As discussed previously, this PR should only copy the phobos implementation. Any improvements should come after this PR is merged.

thewilsonator · 2019-01-04T00:10:33Z

but reformatted it in the process.

Sorry, my editor occasionally does funny things with formatting.

TurkeyMan · 2019-01-04T08:21:18Z

Thx!

Sadly, I just noticed that it doesn't end here :(
I also need to relocate std.algorithm.mutation.move to druntime. Does anyone wanna have a go at that?
Druntime needs all the allocation primitives; emplace, destroy, move... also forward?

TurkeyMan · 2019-01-04T08:22:10Z

I kinda feel like move and forward also belong in core.lifetime...?

thewilsonator · 2019-01-04T08:23:22Z

Sure, its as good a place as any I suppose.

PetarKirov · 2019-01-04T22:44:22Z

+1 for move and forward.

TurkeyMan requested review from MartinNowak and andralex as code owners December 7, 2018 08:12

TurkeyMan force-pushed the move_emplace branch from 007aae6 to ef2bddd Compare December 7, 2018 09:00

TurkeyMan requested review from schveiguy and wilzbach as code owners December 7, 2018 09:00

TurkeyMan force-pushed the move_emplace branch from ef2bddd to 7b1cd79 Compare December 7, 2018 09:12

TurkeyMan force-pushed the move_emplace branch from 7b1cd79 to bec59b4 Compare December 7, 2018 09:36

JinShil mentioned this pull request Dec 9, 2018

Refactor object.d: Move destroy and friends to new file, lifetime.d #2399

Closed

thewilsonator reviewed Dec 13, 2018

View reviewed changes

src/object.d Outdated Show resolved Hide resolved

PetarKirov reviewed Dec 19, 2018

View reviewed changes

src/core/internal/traits.d Show resolved Hide resolved

thewilsonator force-pushed the move_emplace branch from 94c303c to ebe9aae Compare December 31, 2018 07:15

thewilsonator force-pushed the move_emplace branch 2 times, most recently from b709f86 to 4e520aa Compare December 31, 2018 07:54

thewilsonator approved these changes Dec 31, 2018

View reviewed changes

jacob-carlborg suggested changes Dec 31, 2018

View reviewed changes

changelog/emplace.dd Outdated Show resolved Hide resolved

jacob-carlborg previously requested changes Dec 31, 2018

View reviewed changes

Move emplace to core/lifetime.d for symmetry with destroy.

5ceab47

TurkeyMan force-pushed the move_emplace branch from 4e520aa to 5ceab47 Compare January 3, 2019 08:58

TurkeyMan changed the title ~~Move emplace to object.d for symmetry with destroy.~~ Move emplace to druntime for symmetry with destroy. Jan 3, 2019

PetarKirov approved these changes Jan 3, 2019

View reviewed changes

thewilsonator added the auto-merge label Jan 4, 2019

dlang-bot merged commit d945061 into dlang:master Jan 4, 2019

TurkeyMan deleted the move_emplace branch January 4, 2019 18:43

Uh oh!

Move emplace to druntime for symmetry with destroy. #2398

Move emplace to druntime for symmetry with destroy. #2398

Uh oh!

Conversation

TurkeyMan commented Dec 7, 2018

Uh oh!

dlang-bot commented Dec 7, 2018

Bugzilla references

Testing this PR locally

Uh oh!

jacob-carlborg commented Dec 7, 2018

Uh oh!

JinShil commented Dec 7, 2018

Uh oh!

TurkeyMan commented Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TurkeyMan commented Dec 7, 2018

Uh oh!

JinShil commented Dec 7, 2018

Uh oh!

TurkeyMan commented Dec 7, 2018

Uh oh!

schveiguy commented Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TurkeyMan commented Dec 7, 2018

Uh oh!

TurkeyMan commented Dec 7, 2018

Uh oh!

schveiguy commented Dec 7, 2018

Uh oh!

TurkeyMan commented Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JinShil commented Dec 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schveiguy commented Dec 8, 2018

Uh oh!

JinShil commented Dec 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TurkeyMan commented Dec 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JinShil commented Dec 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TurkeyMan commented Dec 9, 2018

Uh oh!

JinShil commented Dec 9, 2018

Uh oh!

jacob-carlborg commented Dec 9, 2018

Uh oh!

schveiguy commented Dec 9, 2018

Uh oh!

TurkeyMan commented Dec 9, 2018

Uh oh!

Uh oh!

wilzbach commented Dec 18, 2018

Uh oh!

PetarKirov commented Dec 19, 2018

Uh oh!

Uh oh!

thewilsonator commented Dec 31, 2018

Uh oh!

Uh oh!

jacob-carlborg left a comment

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg Dec 31, 2018

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg Dec 31, 2018

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg Dec 31, 2018

Choose a reason for hiding this comment

Move `emplace` to druntime for symmetry with `destroy`. #2398

Move `emplace` to druntime for symmetry with `destroy`. #2398

TurkeyMan commented Dec 7, 2018 •

edited

Loading

schveiguy commented Dec 7, 2018 •

edited

Loading

TurkeyMan commented Dec 7, 2018 •

edited

Loading

JinShil commented Dec 8, 2018 •

edited

Loading

JinShil commented Dec 8, 2018 •

edited

Loading

TurkeyMan commented Dec 8, 2018 •

edited

Loading

JinShil commented Dec 8, 2018 •

edited

Loading