Issue 10194. Variant does not call struct destructors.

[yglukhov]

First commit fixes issue 10194
The second commit adds support for elaborate copying.

[DmitryOlshansky]

LGTM

[dnadlinger]

Is this covered by the tests?

[yglukhov]

It's implicitly covered by lots of tests. But i can add an "explicit" test for that, if needed.

[quickfur]

ping
Any update since last comment? This looks like an important fix for structs with dtors.

[yglukhov]

Well, the only update I have, is trying to call opAssign when appropriate appeared to be a really bulky solution, which I'm not proud of, also it has some bugs, and I didn't manage to debug it yet. I would be really happy if you guys accept the current "always-destruct-on-assign" solution. If you think of that as RAII support, it should be almost-the-same semantically. Also opAssign support could be an additional improvement with a separate PR. What do you think?

[mihails-strasuns]

I agree. While missing opAssign support will be rather annoying bug current situation is MUCH worse and this PR improves it.

[mihails-strasuns]

Auto-merge toggled on

[mihails-strasuns]

Still, if you can come up with update PR which adds opAssign support, that will be very appreciated ;)

[yglukhov]

Thanx! Regarding opAssign - I'll try my best =)

[CyberShadow]

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=13300

[CyberShadow]

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=13303

[yglukhov]

Thats a shame. I can suggest the following solutions:

1. Quick and dirty. Make variant.postblit pure, call fptr, assuming pure. Static assert if type's postblit is not pure, effectively forbidding types with non-pure postblits.
2. Revert this PR.
   What do you think?

[9rnsr]

If you want to check that A has an elaborate copy operation, you should use std.traits.hasElaborateCopyConstructor.

[yglukhov]

Check for elaborate copying corrected: 2437

[yglukhov]

The bugs remain open though. The problem is that variant.postblit is not pure. Also, it's not safe and it may throw. Purity, safety and nothrowness are runtime properties of Variant. So maybe it would be safer to always treat variant.postblit as non-pure, @System and throwing, and wrapping it's postblit in assume-blocks where appropriate. And in such case those bugs are no more bugs.

[9rnsr]

@yglukhov I opened PR #2439 to fix the regression, which will conditionally define postblit and destructor.

[mihails-strasuns]

This PR has introduced regression https://issues.dlang.org/show_bug.cgi?id=13871 which is still present in Phobos master.

[mihails-strasuns]

Reduced test case:

import std.variant;
alias A = Algebraic!(int, typeof(null));
struct B { A value; }
alias C = Algebraic!B;

void main()
{
   C var;
   var = C( B() );
}

Crashes since this PR has been merged.
Ping @yglukhov @9rnsr

[yglukhov]

Quick fix: #2826.

[CyberShadow]

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=14062

(This was known since 2015-01-31, I'm only posting here to notify the PR participants)