add unittest attributes to std.algorithm.sorting

[wilzbach]

It's possible that someone introduced new unittests that aren't explicitly @safe or @System, but with the exception of std.stream (because of imminent deprecation) all phobos unittests are now explicity tagged.

@atilaneves how did you check that? grep -r "^unittest" -r std still yields a lot.

[codecov-io]

Current coverage is 88.68% (diff: 100%)

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

@@             master      #4685   diff @@
==========================================
  Files           121        121          
  Lines         73827      73827          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          65471      65472     +1   
+ Misses         8356       8355     -1   
  Partials          0          0          

Powered by Codecov. Last update f75eeb5...ff862f9

[9il]

Why explicit @system is required?

[wilzbach]

Why explicit @System is required?

I agree its kinda pointless, but at least we can then turn on CI checking. For more infos:

http://forum.dlang.org/thread/nkn1g6$b00$1@digitalmars.com?page=1

[9il]

I don't understand. For example, why this should be marked as system?

@system unittest
  {
      int[] a = [ 5, 7, 2, 6, 7 ];
      int[] b = [ 2, 1, 5, 6, 7, 3, 0 ];
     topN(a, b);
     sort(a);
     assert(a == [0, 1, 2, 2, 3]);
  }

[wilzbach]

I don't understand. For example, why this should be marked as system?

Because BinaryHeap (used in topN) isn't @safe, e.g. it uses the unsafe RefCounted.

[jmdavis]

I don't understand. For example, why this should be marked as system?

If that code can't be marked as @safe, we have a serious problem. We still have plenty of issues where stuff can't be marked @safe when we should be able to mark it that way, but if code like that can't be @safe... it's kind of pathetic actually. And marking it as @system seems like it's just hiding the problem. IMHO, it would be far better to not mark it as @system and leave it as-is until it can be @safe. Slapping @system on stuff that should be @safe just seems like a bad idea.

[jmdavis]

Because BinaryHeap (used in topN) isn't @safe, e.g. it uses the unsafe RefCounted.

But all of that is internal to topN. topN should not be @system because of anything it does with BinaryHeap. If it is, then something needs to be done to it so that the appropriate parts are marked as @trusted and topN can be inferred to be @safe like it should be.

[atilaneves]

Because anything that is @System should be explicit so that we can grep for them and try to fix the problem

[atilaneves]

@wilzbach : it's normal for that grep to hit, plenty of the tests have the attribute on the previous line

[9il]

@system unittest is equal to not @safe. So we can grep not @safe unittests .

[atilaneves]

You can grep for non @safe tests, but not easily. Explicit is better than implicit.

[wilzbach]

@ all - I didn't want to start this debate - I thought that it was a agreed practice.

In any case I think we all can agree that just @safe is an improvement - this is what this PR contains now ;-)

but if code like that can't be @safe... it's kind of pathetic actually.

Yes :/

If it is, then something needs to be done to it so that the appropriate parts are marked as @trusted

Well it would be this line, but I don't know whether we can blindly apply @trusted here.

auto heap = () @trusted { return BinaryHeap!(Range1, less)(r1); }();

[9il]

Well it would be this line, but I don't know whether we can blindly apply @trusted here.

auto heap = () @trusted { return BinaryHeap!(Range1, less)(r1); }();

this should be discussed in separate PR

[9il]

Can it be just lambda aliases?

[schveiguy]

It was called for by Walter. Step 1 is to mark all unit tests @safe that can be, or mark them @system otherwise. Then we can look for ones marked @system and fix them.

this should be discussed in separate PR

We can discuss it right here. No, it shouldn't be marked @trusted, as we have no idea what less is going to do and whether it's @safe.

[schveiguy]

e.g. it uses the unsafe RefCounted.

We should fix RefCounted, that should be able to be @safe.

[jmdavis]

We should fix RefCounted, that should be able to be @safe.

Actually, I'm pretty sure that it can't be @safe, which is part of why Walter is working on a solution that adds ref-counting to the language. That being said, I would think that it should be possible for BinaryHeap to verify that its use of RefCounted is @safe and mark the appropriate code as @trusted. I'd have to actually go digging through BinaryHeap to verify that though (the main question being whether there's any possibility of a reference to something that's ref-counted escaping and therefore running into problems when the RefCounted's innards are destroyed and freed - which is the main reason that RefCounted can't be @safe).

[WalterBright]

@atilaneves is correct. Explicitly marking unittests as @safe or @System makes it clear where things are good and where more engineering is needed.

[schveiguy]

Looking at RefCounted, the problem is that we conflate the constructor of the ref counted struct with the construction of the actual ref-counted payload. You can't blindly trust the construction of the ref counted struct unless you know the constructor of the payload is also @safe.

What we need for your idea is a 2-stage construction, which is unwieldy. I had envisioned marking the appropriate construction pieces of RefCounted as @trusted, but @klickverbot is correct that because it returns a reference to the payload blindly, we can't trust it. What we need is a mechanism to ensure the reference only exists as long as the scoped RefCounted struct exists. I thought dip25 would do that, but I don't know if it's ready.