[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