Fix cannot throw scoped exception

[WalterBright]

because that would leak it and cause corruption.

[WalterBright]

Blocking dlang/dmd#6675 which checks for this kind of bug.

[schveiguy]

This is not a bug. The exception is caught inside the scope always. The idea is to avoid any gc activity. Is there a better way to do this?

[WalterBright]

The exception is caught inside the scope always.

Yes, you are right. I missed that.

The idea is to avoid any gc activity. Is there a better way to do this?

Yes. Return an error code (by return value or an out parameter). All DeprecatedCycleException does is flag an error, it doesn't even have a message.

[WalterBright]

Fixed it to use error code for cycles instead of exception.

[WalterBright]

@schveiguy you're the right person to review this

[schveiguy]

I'll take a look. The convenience of the stack unwinding is what I liked about that mechanism.

[schveiguy]

IIRC, when I was developing this "fix" for the deprecated cycle detection, I tried doing what you are doing, and thought the exception mechanism is much cleaner and easier to implement.

Perhaps we should also re-examine whether the GC should be verboten at this stage. I'm wondering if we can just throw an allocated exception, and then delete it in the catch handler. From a philosophical standpoint, we're still using the GC when an actual cycle is detected, but if the program runs as planned, it is @nogc, even if a deprecated cycle is detected. But from a practical standpoint, if we allocate when throwing, and then delete within the catch, it would be pretty much equivalent. I'm not sure what the requirements are this early in the process, perhaps @MartinNowak has some more insight.

In any case, with the requested change, I think this will work.

[schveiguy]

You need to do this a weird way. The old algorithm sets _ctors and _tlsctors internally, so the assignment cannot happen. The exception prevented the assignment. You need to do something like this:

auto ctorTemp = doSort(MIctor | MIdtor, cycle);
if(!cycle)
{
    _ctors = ctorTemp;
    ctorTemp = doSort(MItlsctor | MItlsdtor, cycle);
    if(!cycle)
        _tlsctors = ctorTemp;
}

[WalterBright]

I implemented the suggested behavior, but changed the return to a ref parameter which simplifies the logic.

In general, the GC should not be used in druntime if it can be avoided.

Exceptions are expensive (both to throw and in code size) and should be avoided where reasonable.

[schveiguy]

Optional improvement listed, but this would work as you have it.

[schveiguy]

Hm... what about returning bool, and then if statement below reads:

if (doSort(...) || doSort(...))

In any case, the new code is correct at least.

[WalterBright]

Returning both a bool and a ref with the same value strikes me as odd and a bit eyebrow raising. I'd need a better reason to use it.

[WalterBright]

Let's just get this merged and move on.

[schveiguy]

Not sure what you mean, you just would return a bool. But not important right now.

[ibuclaw]

Indeed, the ref parameter is no longer required if doSort just returns a bool.

[schveiguy]

Auto-merge toggled on