[Trivial] Fix deprecated use of delete(x) in favor of x.destroy()

[dkgroot]

No description provided.

[dlang-bot]

Thanks for your pull request and interest in making D better, @dkgroot! 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.

[schveiguy]

Hm... this isn't the same. destroy is going to do nothing to most of those things, and it certainly isn't going to "exercise each function" in the file.

[dkgroot]

@schveiguy any suggestion on how to get rid of:

src/rt/tracegc.d(43): Deprecation: The delete keyword has been deprecated.  Use object.destroy() instead.
src/rt/tracegc.d(44): Deprecation: The delete keyword has been deprecated.  Use object.destroy() instead.
src/rt/tracegc.d(45): Deprecation: The delete keyword has been deprecated.  Use object.destroy() instead.
src/rt/tracegc.d(46): Deprecation: The delete keyword has been deprecated.  Use object.destroy() instead.
src/rt/tracegc.d(47): Deprecation: The delete keyword has been deprecated.  Use object.destroy() instead.
src/rt/tracegc.d(48): Deprecation: The delete keyword has been deprecated.  Use object.destroy() instead.
src/rt/tracegc.d(49): Deprecation: The delete keyword has been deprecated.  Use object.destroy() instead.
src/rt/tracegc.d(50): Deprecation: The delete keyword has been deprecated.  Use object.destroy() instead.

During compilation of druntime ?

[schveiguy]

Sorry for the lack of response. Indeed, that message is not enough. What it really should say is "you should use destroy, and don't worry about freeing the memory, the GC will do it for you", as that is really the safe route. But often you want to free the memory. So really the answer is destroy(*ptr); GC.free(ptr).

However, in this case, there is no allocation, so you just want to free the (null) pointers. Destroying them does nothing. You will have the same effect as using delete (the functions will be called), and the warning should go away.

[dkgroot]

@schveiguy Thanks for the clarification !

[schveiguy]

Filed an issue about the diagnostic message: https://issues.dlang.org/show_bug.cgi?id=18505

[schveiguy]

As you may notice there, I forgot there is actually a new function that does the same as delete: https://dlang.org/phobos-prerelease/core_memory.html#.__delete,

But I think at this point, just calling GC.free should be enough here.

[dkgroot]

@schveiguy Are you going to fix/update druntime to use either one of these two solutions ?

[schveiguy]

Possibly, but you could do it as well.

[JinShil]

Please note that __delete was created to be used as a last resort when there is no other alternatives. See https://dlang.org/changelog/pending.html#deprecate_delete

Please use destroy() if possible, and GC.free() if required.

UPDATE: Also, all instances of delete in tracegc.d are in a version(none) block, so how is it being called?

[JinShil]

Also, all instances of delete in tracegc.d are in a version(none) block, so how is it being called?

OK, I remember now. It's because delete, the keyword, has been deprecated, so the deprecation message is emitted by the parser.

[dkgroot]

@JinShil Good question, don't have an answer for it though. Please compile druntime manually to see them popping up. Maybe deprecation does not care about version(none) ??

[schveiguy]

@JinShil destroy does nothing here. You can call it, sure, but none of the targets have been allocated.

Also, all instances of delete in tracegc.d are in a version(none) block,

That concerns me a bit as well, as any changes we make will not actually be tested. But at this point, I'm not worried about this code, we should just get rid of the deprecation warning.

[JinShil]

destroy does nothing here

Does delete do anything here either? It looks like replacing delete with destroy would do the same thing...nothing.

Also, it looks like that block of code is just a test to exercise each function. Due to the fact that it is in a version(none) block, we could comment it out and we'd still be keeping the status quo.

[schveiguy]

Actually, looking at the disassembly, delete calls the appropriate runtime functions (e.g. _d_delarray_t, _d_delclass). __delete doesn't do this (I get a segfault), and GC.free isn't going to do this properly.

So I would recommend, we just comment this whole thing out. The only other alternative is to directly call the runtime functions, which is such a low level detail that I don't think it's reasonable to assume they will not change in the future. The version(none) makes me just think, let's comment this whole thing out, and not worry about it right now.

ping @WalterBright as he seems to have committed this code 3 years ago.

BTW, since the commit has gone away, there is no real reference to what we are talking about. Here are the offending lines:

druntime/src/rt/tracegc.d

Lines 43 to 50 in 2d87bb9

	int[] a; delete a;
	S[] as; delete as;
	C c; delete c;
	I i; delete i;
	C* pc = &c; delete *pc;
	I* pi = &i; delete *pi;
	int* pint; delete pint;
	S* ps; delete ps;

[wilzbach]

BTW you might also be interested in #1982 which adds -de to druntime, but I had a rather hard-time fixing the deprecations in the new vectorized array operations.

[wilzbach]

Actually, looking at the disassembly, delete calls the appropriate runtime functions (e.g. _d_delarray_t, _d_delclass). __delete doesn't do this (I get a segfault)

Interesting. When I first implemented __delete, I went with calling the runtime functions, but then changed it in course of the review at #2037

I assumed that GC.free checks for null pointers, but apparently it doesn't.
We should fix that -> #2108

So I would recommend, we just comment this whole thing out.

👍