HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Properly destroying the VBE

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
properlythevbedestroying

Problem

The disposable wrappers worked exactly as intended... and that turned out being a huge mistake:

That's because the .net runtime creates a Runtime Callable Wrapper per-type, not per-instance*; it took a number of refactorings and adjustments just to be able to get the project to build and run, and then over 900 tests were broken and my life was a nightmare.

Then I had an idea: since .net wouldn't let me destroy objects that were otherwise ready to be collected, what if I ditched IDisposable and moved on to a hierarchical teardown strategy?

Enter ISafeComWrapper:

namespace Rubberduck.VBEditor.SafeComWrappers
{
    public interface ISafeComWrapper
    {
        /// 
        /// Releases all COM objects.
        /// 
        void Release();
    }
}


In the Extension class (which you may recall seeing here), we Release the entire VBE object graph, like this:

private void ShutdownAddIn()
{
    if (_app != null)
    {
        _app.Shutdown();
        _app = null;
    }

    if (_kernel != null)
    {
        _kernel.Dispose();
        _kernel = null;
    }

    _ide.Release();
    _isInitialized = false;
}


So here's the SafeComWrapper base class:

```
using System;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.InteropServices;

namespace Rubberduck.VBEditor.SafeComWrappers
{
public abstract class SafeComWrapper : ISafeComWrapper, IEquatable>
where T : class
{
protected SafeComWrapper(T comObject)
{
_comObject = comObject;
}

public abstract void Release();

private readonly T _comObject;
public T ComObject { get { return _comObject; } }
public bool IsWrappingNullReference { get { return _comObject == null; } }

protected TResult InvokeResult(Func member)
{
try
{
return member.Invoke();
}
catch (COMException exception)
{
throw new Wrapper

Solution

public static bool operator ==(SafeComWrapper a, SafeComWrapper b)
{
    if (ReferenceEquals(a, null) && ReferenceEquals(b, null))
    {
        return true;
    }
    return !ReferenceEquals(a, null) && a.Equals(b);
}


I think that this should be done a little differently, this looks overly complicated and a bit confusing.

I was thinking that a ternary statement would make this much cleaner and less redundant with the ReferenceEquals(a, null)

it would look something like this.

public static bool operator ==(SafeComWrapper a, SafeComWrapper b)
{
    return ReferenceEquals(a, null) ? ReferenceEquals(b, null) : a.Equals(b);
}

Code Snippets

public static bool operator ==(SafeComWrapper<T> a, SafeComWrapper<T> b)
{
    if (ReferenceEquals(a, null) && ReferenceEquals(b, null))
    {
        return true;
    }
    return !ReferenceEquals(a, null) && a.Equals(b);
}
public static bool operator ==(SafeComWrapper<T> a, SafeComWrapper<T> b)
{
    return ReferenceEquals(a, null) ? ReferenceEquals(b, null) : a.Equals(b);
}

Context

StackExchange Code Review Q#143358, answer score: 2

Revisions (0)

No revisions yet.