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

JinShil · 2018-12-09T00:22:27Z

Reboot of #2222 in an attempt to help with #2398

dlang-bot · 2018-12-09T00:22:28Z

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

thewilsonator · 2018-12-09T01:03:55Z

/var/lib/buildkite-agent/builds/buildkite-agent-04-1/dlang/druntime/build/distribution/bin/../imports/object_/lifetime.d(446): Deprecation: `object.isnan` is not visible from module `lifetime`
--
  | /var/lib/buildkite-agent/builds/buildkite-agent-04-1/dlang/druntime/build/distribution/bin/../imports/object_/lifetime.d(446): Error: function `object.isnan` is not accessible from module `lifetime`

JinShil · 2018-12-09T01:07:00Z

Thanks, @thewilsonator

thewilsonator · 2018-12-09T01:07:39Z

No problems.

JinShil · 2018-12-09T01:50:34Z

The auto-tester was failing, so I tried to modify some of the makefiles. Now circleci is failing. I don't know why. If someone has an idea, please let me know.

JinShil · 2018-12-09T02:26:03Z

posix.mak

 endif

 # macro that returns the module name given the src path
-moduleName=$(subst rt.invariant,invariant,$(subst object_,object,$(subst /,.,$(1))))


I think this is a string replacement from a bygone era, so I removed it. But I'm not even sure if I did it correctly because I generally don't work with makefiles, and dread it every time I have to.

JinShil · 2018-12-09T02:32:20Z

What is this all about, and why is it affecting this PR?

compilable/json.d(185): Deprecation: `extern(Pascal)` is deprecated. You might want to use `extern(Windows)` instead.

EDIT: Nevermind. I see it's actually complaining about the sanitized json diff. Looks like this needs a change to the DMD repository.

EDIT: This PR now needs dlang/dmd#9049

JinShil · 2018-12-09T03:32:47Z

WARNING: This PR causes a huge change in the documentation; see DAutoTest, but I can't see the usual diffs to determine what the changes are. I browsed the documentation and couldn't find any issues, but that doesn't mean there aren't any.

EDIT: Looks like destroy is missing from object now: http://dtest.dlang.io/artifact/website-0f10794ab3a1dd660a6c9969e522088fb4f89147-17f397fbca460634f6544e1ba7bbafa0/web/phobos-prerelease/object.html
I'm not yet sure how to fix that.

JinShil · 2018-12-09T05:00:40Z

I don't understand what's failing in DAutoTest. @CyberShadow could you take a quick look? Or @adamdruppe, is that your code?

EDIT: Nevermind. I think the real error was further down in the wall of output text. I think I figured it out.

JinShil · 2018-12-09T06:13:09Z

Ok, this is looking good now. Please notice the changes in the documentation. There's a new object_ package in the left navigation and that's why there are so many changed files.

There was a concern in #2222 about this negatively affecting build times. I couldn't see any measurable difference when building phobos (at least with all the statistical noise). Reviewers are advised to independently verify.

JinShil · 2018-12-09T06:27:22Z

If this is accepted, I'll do some of the others. I'm thinking...

object_/array.d - dynamic array support
object_/typeinfo.d - all the TypeInfo classes and maybe even ModuleInfo
object_/associativeArray.d - associative array support
object_/comparison.d - __cmp and __equals

CyberShadow · 2018-12-09T06:30:42Z

src/object_/lifetime.d

+ * Contains utilities for managing lifetimes.
+ *
+ * This module is not intended to be imported or used directly.  It is
+ * publicly imported by object.d, and should be accessed through there.


What does "should be accessed through there" mean? Would it be more accurate to say that the declarations in this module are automatically available in all D modules, like those in object?

This module is not intended to be imported or used directly. It...

"It" refers to "This module".

It is publicly imported by object.d, and should be accessed through there.

"there" refers to "object.d"

This is natural English for me (using pronouns to refer to previously mentioned things). If you'd like me to change it, please send me copy-pastable text.

What I'm trying to say is that it might not be obvious to users that they don't have to import anything in order to use the symbols from that module. E.g. someone googles for "dlang destroy object", finds this page on dlang.org, sees that it's in a module, and tries importing it. Heck, at that point the user might not even know that even object doesn't need to be imported.

"accessed through there" also can be interpreted that something needs to be done by the user to achieve this, specifically something involving what "there" refers to (the object module).

I gave it another go to address your concerns. I don't know if it's good enough. Please send me something better it's not.

It's better :)

TurkeyMan · 2018-12-09T08:09:53Z

mak/DOCS

 	$(DOCDIR)\object.html \
+    \
+    $(DOCDIR)\object__lifetime.html \
+    \


Spaces where should be tabs?

I'm not sure about this location, but make can be fussy about tabs vs spaces.

TurkeyMan · 2018-12-09T08:11:44Z

Why object_/lifetime.d, and not just lifetime.d? That underscore looks pretty janky for such a fundamental package?

JinShil · 2018-12-09T08:32:23Z

Why object_/lifetime.d, and not just lifetime.d?

There were concerns in #2222 about placing lifetime.d in the root and preventing users from being able to use lifetime.d as their own top level package. I'm actually not sure about that. @wilzbach, care to clarify?

That underscore looks pretty janky for such a fundamental package?

Actually, I just realized what @schveiguy was saying in #2222. I think I should have done core.internal.object all along. I think I'll try that. It will prevent a lot of the other changes I made as well.

TurkeyMan · 2018-12-09T08:38:57Z

preventing users from being able to use lifetime.d as their own top level package

So, core.lifetime? What's what I would have assumed.

JinShil · 2018-12-09T08:57:13Z

So, core.lifetime? What's what I would have assumed.

I think we could make that work, but I'd like to keep all the publicly imported parts of object.d together as a group and segregated from the other core files. I'd be ok with it but I'm going with core.internal.object.lifetime.d until I find a reason not to.

TurkeyMan · 2018-12-09T09:08:01Z

So we're gonna publicly import emplace too then? Because it's an impenetrable module name otherwise.

JinShil · 2018-12-09T09:10:50Z

So we're gonna publicly import emplace too then?

If emplace is placed alongside destroy in lifetime.d it would be imported publicly through object.d. Noone should ever import lifetime.d directly.

TurkeyMan · 2018-12-09T09:23:07Z

why does destroy need to be implicitly available? is it possible to deprecate that public import with time?

JinShil · 2018-12-09T09:30:48Z

why does destroy need to be implicitly available?

I don't know, but now that it is, it'll take a deprecation cycle and a lot of persuasions to change.

is it possible to deprecate that public import with time?

I suppose, if you can convince the right people. There are a lot of architecture problems with druntime.

JinShil · 2018-12-09T09:41:59Z

Damn! If you see the autotester, it's building with 2.079 which doesn't yet have the fix from https://issues.dlang.org/show_bug.cgi?id=19014. I'm sure what to do about that yet. 2.083 (the version on my local machine) also seems to still have the problem. When I build from HEAD everything works fine. @RazvanN7 @wilzbach wasn't that fix supposed to be released already?

EDIT: Looks like the fix is coming 2.084: https://dlang.org/changelog/pending.html

adamdruppe · 2018-12-09T13:23:52Z

so note that none my my code are ran as part of the tests. it is just that the phobos makefile has a build rule with a similar name to my site.

n8sh · 2018-12-09T14:05:36Z

Currently core.internal.* modules don't appear in the documentation. We could keep it that way and just add this in object.d:

version(D_Doc)
{
    /**
    Destroys the given object and optionally resets to initial state. It's used to
    _destroy an object, calling its destructor or finalizer so it no longer
    references any other objects. It does $(I not) initiate a GC cycle or free
    any GC memory.
    If `initialize` is supplied `false`, the object is considered invalid after
    destruction, and should not be referenced.
    */
    void destroy(bool initialize = true, T)(ref T obj)
    if (!is(T == class) && !is(T == interface))
    {
        import lifetime = core.internal.object.lifetime;
        lifetime.destroy!(initialize)(obj);
    }
    /// ditto
    void destroy(bool initialize = true, T)(T obj)
    if (is(T == class) || is(T == interface))
    {
        import lifetime = core.internal.object.lifetime;
        lifetime.destroy!(initialize)(obj);
    }
}
else
{
    public import core.internal.object.lifetime : destroy;
}

jacob-carlborg · 2018-12-09T21:22:51Z

src/core/internal/object/lifetime.d

+/**
+ * Contains utilities for managing lifetimes.
+ *
+ * This module is not intended to be imported or used directly.  It is


All public symbols should be package, then the user cannot use this module directly. Assuming that works since object doesn't live in a package.

If I make them package then they won't be visible outside of core.internal.object, right? If that's the case, then I won't be able to publicly import them into object.d.

This goes along with my original idea in #2222 to put them all in the root, rename object.d to package.d and then modify the compiler to look for package.d (or the root of the runtime) instead of object.d. Then the symbols can all be made package.

There were concerns about not being able to use lifetime.d in one's own file hierarchy. But, honestly, I don't understand that, because they would be under a different folder and users should never import the runtime's lifetime.d anyway.

It's possible to specify a symbol along with package, i.e. package(core) which would make it accessible to the whole core package and all its sub packages. But since object lives outside any package, that might not work anyway.

JinShil · 2018-12-09T23:26:42Z

Currently core.internal.* modules don't appear in the documentation.

Yes, I'm aware and working on that. My current workaround is https://github.com/dlang/druntime/pull/2399/files#diff-a68e58fcf0de5aa198fcaceafe4e8cf9R6 That's a statement in object.d's documentation comment that reads...

* This module publicly imports symbols from $(REF ,core,internal,object,lifetime).

... if I can only get the link to work right (If you know the correct DDoc syntax for the link, please do tell). You can see it rendered here.

I don't really want to do your version(D_Doc) workaround because the documentation should accompany the implementation in lifetime.d and we'd have to duplicated in both places, thus trading one maintenance headache for another.

I also tried mixin(import("core.internal.object.lifetime.d")), but the compiler didn't generate the docs then either.

There's actually an issue report already documenting that this isn't working right: https://issues.dlang.org/show_bug.cgi?id=10665 I'm going to try to fix that tonight. If I'm successful, the problem will just go away and we'll all have one less annoyance to deal with in our own code.

JinShil · 2018-12-16T22:21:35Z

Due to Issue 19014 This needs the autotester upgraded to 2.084 before it can move forward. Closing until that becomes reality.

JinShil requested review from MartinNowak, andralex and wilzbach as code owners December 9, 2018 00:22

thewilsonator approved these changes Dec 9, 2018

View reviewed changes

thewilsonator added the auto-merge label Dec 9, 2018

dlang-bot removed the auto-merge label Dec 9, 2018

thewilsonator added the auto-merge label Dec 9, 2018

dlang-bot removed the auto-merge label Dec 9, 2018

JinShil commented Dec 9, 2018

View reviewed changes

JinShil mentioned this pull request Dec 9, 2018

Modify sanitize_json.d to support new runtime directory object_ dlang/dmd#9049

Merged

CyberShadow reviewed Dec 9, 2018

View reviewed changes

TurkeyMan reviewed Dec 9, 2018

View reviewed changes

JinShil requested a review from schveiguy as a code owner December 9, 2018 08:40

JinShil mentioned this pull request Dec 9, 2018

Fix Issue 19014 - Compiler imports symbols that aren't actually imported dlang/dmd#9050

Merged

jacob-carlborg reviewed Dec 9, 2018

View reviewed changes

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

c72630a

JinShil closed this Dec 16, 2018

Uh oh!

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

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

Uh oh!

Conversation

JinShil commented Dec 9, 2018

Uh oh!

dlang-bot commented Dec 9, 2018

Bugzilla references

Testing this PR locally

Uh oh!

thewilsonator commented Dec 9, 2018

Uh oh!

JinShil commented Dec 9, 2018

Uh oh!

thewilsonator commented Dec 9, 2018

Uh oh!

JinShil commented Dec 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

JinShil commented Dec 9, 2018

Uh oh!

JinShil commented Dec 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TurkeyMan commented Dec 9, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

JinShil commented Dec 9, 2018

Uh oh!

TurkeyMan commented Dec 9, 2018

Uh oh!

JinShil commented Dec 9, 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!

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

Uh oh!

Uh oh!

adamdruppe commented Dec 9, 2018

Uh oh!

n8sh commented Dec 9, 2018

Uh oh!

jacob-carlborg Dec 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

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

JinShil commented Dec 9, 2018 •

edited

Loading

JinShil commented Dec 9, 2018 •

edited

Loading

JinShil commented Dec 9, 2018 •

edited

Loading

CyberShadow Dec 9, 2018 •

edited

Loading

JinShil commented Dec 9, 2018 •

edited

Loading

TurkeyMan commented Dec 9, 2018 •

edited

Loading

JinShil commented Dec 9, 2018 •

edited

Loading

JinShil commented Dec 9, 2018 •

edited

Loading

jacob-carlborg Dec 9, 2018 •

edited

Loading

JinShil Dec 9, 2018 •

edited

Loading

JinShil commented Dec 9, 2018 •

edited

Loading