destroy() receives initialize argument

[TurkeyMan]

Fixes Issue 19214 - Support object.destruct() for efficient (and correct!) destruction

Implement via template argument to destroy as discussed in #2292

[dlang-bot]

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

Auto-close	Bugzilla	Severity	Description
✓	19214	enhancement	Support object.destruct() for efficient (and correct!) destruction

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#2376"

[thewilsonator]

The only thing I'm slightly concerned with is destroy with static arrays with the explicit template tinsansiation, but I suppose that can't be helped.

[thewilsonator]

Then perhaps call it reinitialize (or maybe reinit_memory)?

[TurkeyMan]

I considered reinitialize, I decided against it. The other one feels unnecessarily wordy to me.
3rd opinions anyone?

Code and comments in harmony

[thewilsonator]

generated/linux/release/64/benchmark: symbol lookup error: generated/linux/release/64/benchmark: undefined symbol: _D6object__T7destroyTS3std4file15DirIteratorImplZQBlFNfKQBjZv
--
  | posix.mak:333: recipe for target 'benchmark-compile-only' failed
  | make: *** [benchmark-compile-only] Error 127

I' assuming this is unrelated.

[TurkeyMan]

I presumed so...

[thewilsonator]

Hmm, that demangles as @safe void object.destroy!(std.file.DirIteratorImpl).destroy(ref std.file.DirIteratorImpl), looks like something is trying to use the old definition somewhere.

[thewilsonator]

Hmm, https://github.com/dlang/druntime/blob/master/benchmark/runbench.d#L58 I don't see an explicit call to destroy there, maybe the compiler is inserting it?

[TurkeyMan]

Static lib? Pre-compiled code? Left-over binary object that wasn't rebuild from fail dirty detection in CI step?

I have audited every call to destroy in dlang org, nothing. No generated/implicit calls from within DMD either.

[thewilsonator]

IDK, giving it another kick now.

[thewilsonator]

Nope still fails, thats just weird! SO mentions that symbol lookup error is to do with shared libraries. It does seem odd that the make target benchmark-compile-only is issued in the phase "building phobos". Looks like it could be linking the wrong dynamic library, those paths don't look consistent:

DMD=../dmd/generated/linux/release/64/dmd generated/linux/release/64/benchmark --repeat=0 --dflags="-conf= -m64 -I/var/lib/buildkite-agent/builds/buildkite-agent-06-1/dlang/druntime/build/dlang-druntime/druntime/import -I../phobos -L-L../phobos/generated/linux/release/64 -fPIC -defaultlib=libphobos2.so -L-rpath=../phobos/generated/linux/release/64 -de"
--
  | compiler: ../dmd/generated/linux/release/64/dmd -conf= -m64 -I/var/lib/buildkite-agent/builds/buildkite-agent-06-1/dlang/druntime/build/dlang-druntime/druntime/import -I../phobos -L-L../phobos/generated/linux/release/64 -fPIC -defaultlib=libphobos2.so -L-rpath=../phobos/generated/linux/release/64 -de
  | generated/linux/release/64/benchmark: symbol lookup error: generated/linux/release/64/benchmark: undefined symbol: _D6object__T7destroyTS3std4file15DirIteratorImplZQBlFNfKQBjZv
  | posix.mak:333: recipe for target 'benchmark-compile-only' failed
  | make: *** [benchmark-compile-only] Error 127

[thewilsonator]

Please rebase

[thewilsonator]

So rather unhelpfully in the case of a shared build it doesn't tell you what object references the symbol. Building it locally on OSX i get:
$ make -f posix.mak benchmark-compile-only | ddmangle

Undefined symbols for architecture x86_64:
  "@safe void object.destroy!(true, std.file.DirIteratorImpl).destroy(ref std.file.DirIteratorImpl)", referenced from:
      void std.typecons.RefCounted!(std.file.DirIteratorImpl, 0).RefCounted.__dtor() in benchmark.o

$nm path/to/benchmark.o | grep DirIteratorImpl | ddmangle

...
S void std.typecons.RefCounted!(std.file.DirIteratorImpl, 0).RefCounted.__dtor()
...
U @safe void object.destroy!(true, std.file.DirIteratorImpl).destroy(ref std.file.DirIteratorImpl)

So, dirEntries returns a DirIterator which holds a RefCounted!(DirIteratorImpl, RefCountedAutoInitialize.no) whose destructor of uses destroy on the DirIteratorImpl and emits a reference to it.

[thewilsonator]

This is definitely a stale build or wrong lib problem I just make cleaned phobos and reran make -f posix.mak benchmark-compile-only and it worked.

[thewilsonator]

(you'll need to give this a kick to revive the doc tester)

[thewilsonator]

Ahh, that'd be because benchmark-compile-only has no dependency on phobos!

$(ROOT)/benchmark: benchmark/runbench.d target $(DMD)
	$(DMD) $(PHOBOS_DFLAGS) -de $< -of$@

benchmark: $(ROOT)/benchmark
	$<

benchmark-compile-only: $(ROOT)/benchmark $(DMD)
	DMD=$(DMD) $< --repeat=0 --dflags="$(PHOBOS_DFLAGS) -de"

[wilzbach]

While the build script definitely should be improved/fixed, Buildkite always initially builds the latest druntime + phobos version and thus the "global" version of Phobos on Buildkite should be equal to the PR's one. I will look into improving the build script, but in the meantime I think the Buildkite error might originate from it checking out master instead of the PR's version of druntime. I will look more into this.

[thewilsonator]

Thanks!

[thewilsonator]

I think the Buildkite error might originate from it checking out master instead of the PR's version of druntime

Hmm now that you mention it, the symbol that was missing was destroy!(type) (master) not destroy!(bool,type) (this PR) which would make sense.

[wilzbach]

In the meantime I think the Buildkite error might originate from it checking out master instead of the PR's version of druntime. I will look more into this.

-> dlang/ci#344

[thewilsonator]

@wilzbach apparently that didn't fix it.

[wilzbach]

I'm aware and currently working on dlang/ci#351

[thewilsonator]

I can't tell if that fixed that problem, now dlang/druntime and dlang/phobos fail with

sed s/TESTS+=rt_trap_exceptions_drt_gdb// -i druntime/test/exceptions/Makefile
--
  | sed: can't read druntime/test/exceptions/Makefile: No such file or directory

[wilzbach]

Yet another attempt: dlang/ci#352

[wilzbach]

(Though of course as these Buildkite failures are unrelated this can be merged, this shouldn't block this PR)