chore(sumtype): avoid quadratic memory complexity on opEquals

[ljmf00]

Signed-off-by: Luís Ferreira [email protected]

---

Due to match template being quadratic/exponential in terms of memory complexity, (-vtemplate reports a rediculous amount of templates) using opEquals will generate a lot of unnecessary template instances. Removing match here to prevent that.

[dlang-bot]

Thanks for your pull request, @ljmf00!

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.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8607"

[pbackus]

I would much, much rather improve match itself than avoid using it. Duplication like this increases maintenance burden and risks introducing bugs (in fact, I think it has introduced one already).

One simple optimization we can do is to replace the lambda handler in opEquals with a global function, so that instantiations will be shared between different SumType instances. This will also cut down on instantiations of canMatch.

[RazvanN7]

@ljmf00 The style checker fails:

Enforce whitespace before opening parenthesis
--
  | grep -nrE "\<(for\|foreach\|foreach_reverse\|if\|while\|switch\|catch\|version)\(" $(find etc std -name '*.d') ; test $? -eq 1
  | std/sumtype.d:717:            final switch(tag)
  | std/sumtype.d:719:                static foreach(idx, T; Types)
  | std/sumtype.d:724:                            static foreach(ridx, U; typeof(rhs).Types)
  | make: *** [posix.mak:587: style_lint_shellcmds] Error 1

• Mind @pbackus 's comment.

[pbackus]

As I said in my previous comment: code duplication like this adds to our maintenance burden, and increases the risk of introducing bugs, like the one pointed out in my review comment.

I would strongly prefer it if we could try a less drastic alternative, like replacing the lambda with a global function (as has already been done in SumType's destructor), before resorting to this.

Finally, if the justification for this change is memory usage, it would be nice to see before-and-after measurements demonstrating the reduction in memory usage. In spite of what the PR title says, both the original and new code have quadratic complexity.

[pbackus]

This line introduces a bug. In the original version, the following unit test passes:

@safe unittest
{
    int* p = null;
    const int* cp = null;

    SumType!(int*, const(int*)) x = p;
    SumType!(int*, const(int*)) y = cp;

    assert(x != y);
}

With this PR, the unit test above will fail.

[pbackus]

Update: I tried replacing the lambda with a global function in a local git branch. Unfortunately, for reasons I don't fully understand, this ended up causing forward reference errors for recursive sum types that have This[] as one of their member types. So it looks like that approach is off the table, at least for now.

[LightBender]

@pbackus In that case would you recommend moving forward as-is? I know that code duplication is bad, but quadratic memory complexity is worse IMO.

[pbackus]

@LightBender I would be ok with moving forward as long as

1. the bug I pointed out is fixed, and
2. we have before-and-after measurements demonstrating the reduction in memory usage.

As I've pointed out before, both the original and the new version have quadratic complexity, which means that the gain from switching to static foreach is "only" a constant-factor reduction. That doesn't mean it isn't worth it, but it does mean I'm going to need some actual data to convince me.