Remove all uses of the delete keyword and enable the Travis check

[JackStouffer]

1. delete is scheduled for deprecation
2. delete is unsafe: fix Issue 16195 - delete should be @system dmd#5887
3. the GC should be the one to decide when things should be freed

[quickfur]

Can probably remove the FIXME. :-)

[JackStouffer]

@quickfur fixed

[quickfur]

While I applaud the push to rid ourselves of delete, I'm a bit worried about the intentions of the deletes that were removed. They seem to be clear cases of when something is known to be no longer referenced, and can therefore be disposed of right away. With this change, we are now leaving this known garbage behind for the GC to later repeat the work of discovering that this is garbage. This adds to GC load, which seems to be a bad idea given the current GC's performance issues.

Perhaps this doesn't really matter in the long run and we should just go ahead and kill off delete, but I'd like my fears assuaged before we merge this. :-)

One thought that occurred to me is to replace delete with GC.free, but the docs say that if finalization is desired delete should be used instead, which is not promising. Furthermore, this will continue requiring @system for these functions, which may be a problem if the ultimate intention is to make these functions usable from @safe code.

[JackStouffer]

They seem to be clear cases of when something is known to be no longer referenced, and can therefore be disposed of right away.

If you can prove that, then honestly they should be using malloc/free and mark the function as @trusted.

One thought that occurred to me is to replace delete with GC.free, but the docs say that if finalization is desired delete should be used instead, which is not promising.

Most of these are arrays, so no finalization needed.

[andralex]

Yeah, this can't go through, it's a net pessimization. Will insert a few specific comments.

[andralex]

Here it's known to the engineer (i.e. you) that in case of failure no trace of result will be visible from the program. So it's safe to deallocate it. Since it's not really typed, you may use GC.free(result) within a @trusted lambda.

[andralex]

We deprecated delete because a keyword is excessive for an unsafe operation. It's a statement more than anything else. We should instead add a template function unsafeDelete that calls the destructor (if any) and then GC.free. Then we can use that function where we deem fit.

[quickfur]

There's GC.runFinalizers, though I'm not sure if there are any caveats associated with it.

[WalterBright]

I don't agree with the pessimization either.

[PetarKirov]

We should instead add a template function unsafeDelete that calls the destructor (if any) and then GC.free

Why not use GCAllocator.instance.dispose(obj), instead? I think it should be a drop-in replacement for delete.

[JackStouffer]

I will make the suggested changes.

We deprecated delete because a keyword is excessive for an unsafe operation.

I would just like to point out that delete is only marked for eventual deprecation and is not actually deprecated yet, https://issues.dlang.org/show_bug.cgi?id=9433

[JackStouffer]

For most of the allocations in std/internal/math/biguintcore.d I can't use malloc/free and the functions are currently marked pure

[JackStouffer]

@andralex @WalterBright Fixed

[quickfur]

Looks much better, thanks!

Makes me wonder, though, if some of these should use scope(exit) instead? Not that it makes a difference semantically, but might be good for the code to be a better example of idiomatic D. Just a thought.

[JackStouffer]

@quickfur Any ideas on the windows failures?

[JackStouffer]

Rebased

[codecov-io]

Current coverage is 88.70% (diff: 50.00%)

Merging #4604 into master will increase coverage by <.01%

@@             master      #4604   diff @@
==========================================
  Files           121        121          
  Lines         73916      73915     -1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          65565      65565          
+ Misses         8351       8350     -1   
  Partials          0          0          

Powered by Codecov. Last update eb9e676...a8c0682

[wilzbach]

Any ideas on the windows failures?

That's pretty weird:

std.file.FileException@std\file.d(732): C:\cygwin\tmp\deleteme.dmd.unittest.pid16148-testing.txt: The process cannot access the file because it is being used by another process.
----------------
0x005DFFF0 in @trusted int std.file.cenforce!(int).cenforce(int, const(char)[], const(wchar)*, immutable(char)[], uint)
0x00918090 in void std.mmfile.__unittestL634_624()
0x01130EA7 in int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)).__lambda2(immutable(object.ModuleInfo*))

std.windows.syserror.WindowsException@std\mmfile.d(233): CreateFileW: The process cannot access the file because it is being used by another process. (error 32)
----------------
0x0091FD65 in @safe bool std.windows.syserror.wenforce!(bool, immutable(char)[]).wenforce(bool, lazy immutable(char)[], immutable(char)[], uint)
0x00917300 in std.mmfile.MmFile std.mmfile.MmFile.__ctor(immutable(char)[])
0x01130EA7 in int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)).__lambda2(immutable(object.ModuleInfo*))
The system cannot find the path specified.

--- errorlevel 1

[JackStouffer]

I really don't understand where this race condition is coming from. Any help would be great.

[schveiguy]

@JackStouffer an idea is possibly to split this into one PR per module, then figure out which one is causing the error.

Note that GC.free does not do exactly the same thing as delete. delete will actually call the destructor on each of the members of an array, for instance, before freeing it. This is the only possible way I can see that there is a difference here. The only thing you are destroying that might have a meaningful destructor seems to be theBigDigit arrays. I'd suspect the issue is there somehow.

[JackStouffer]

Thanks @schveiguy the problem was destruction.

Ready to go

[schveiguy]

LGTM, I didn't notice the MmFile usage! Errors should have made this obvious :)

[JackStouffer]

Pulling due to approval by team member

[JackStouffer]

Auto-merge toggled on