[ofw] NDv2 provider patch

Hefty, Sean sean.hefty at intel.com
Mon Jul 26 11:11:11 PDT 2010


IMO, the auto-ref class needs to go.

> +class CNDMemoryRegion : public INDMemoryRegion, private CNDBase
>  {
> +       auto_ref<CNDAdapter> m_pAdapter;
> +       WV_MEMORY_KEYS           m_Keys;
> +       VOID* volatile           m_pOv;
> +
> +private:
> +       CNDMemoryRegion(CNDAdapter& adapter);
> +       ~CNDMemoryRegion(){};
> +
>  public:

Does anyone have a clue what exactly the following code does by reading it?

> +       static STDMETHODIMP
> +       CreateInstance(CNDAdapter& adapter, VOID** ppMemoryRegion)
> +       {
> +               auto_ref<CNDMemoryRegion> mr(new CNDMemoryRegion(adapter));
> +               if (mr.get() == NULL) {
> +                       return ND_NO_MEMORY;
> +               }
> +
> +               *ppMemoryRegion = mr.detach();
> +               return ND_SUCCESS;
> +       }

*IF* I'm reading it correctly, this allocates a pointer.  If the allocation succeeds, an 'automatic reference' is taken on it, although instead of just calling Ref(), we use auto_ref<>.  We then call mr.get(), which checks to see if the mr actually exists.  So, eventhough mr.get() looks like it could dereference a NULL mr, the check is provided by some underlying class.  Finally, if mr is valid, then we "detach" it, and return whatever that value returns.

> +       static STDMETHODIMP
> +       CreateInstance(CNDAdapter& adapter, VOID** ppMemoryRegion)
> +       {
> +               auto_ref<CNDMemoryRegion> mr(new CNDMemoryRegion(adapter));

+       return ::HeapAlloc(WinVerbs::NetworkDirect::g_hHeap, 0, size);

> +               if (mr.get() == NULL) {
> +                       return ND_NO_MEMORY;
> +               }
> +
> +               *ppMemoryRegion = mr.detach();
> +               return ND_SUCCESS;
> +       }

Contrast the above code with what was there:

	CreateInstance(CNDAdapter *pAdapter, INDMemoryRegion** ppMemoryRegion)
	{
		HRESULT hr;
		CNDMemoryRegion *mr;

		mr = new CNDMemoryRegion(pAdapter);
		if (mr == NULL) {
			hr = ND_NO_MEMORY;
			goto err;
		}

		*ppMemoryRegion = mr;
		return ND_SUCCESS;

	err:
		*ppMemoryRegion = NULL;
		return hr;
	}

which is way more explicit and simpler to read/understand.  I see no value in the auto_ref obscurity and find that it makes the code more difficult to maintain, not easier.



More information about the ofw mailing list