[ofw] NDv2 provider patch

[Fab Tillier]

Hefty, Sean wrote on Mon, 26 Jul 2010 at 11:11:11
> 
> 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.

It allocates a reference counted object.  That's much more specific than just 'a pointer.'

> If the
> allocation succeeds, an 'automatic reference' is taken on it, although
> instead of just calling Ref(), we use auto_ref<>.

No, the object (and the reference implicit in creation) is stored in an auto_ref<> container.

>  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,

'mr' is an auto_ref container.  It is on the stack, and cannot itself be NULL.

> the
> check is provided by some underlying class.  Finally, if mr is valid,
> then we "detach" it, and return whatever that value returns.

You explicitly hand the reference that was stored in the auto_ref container to the user.

>> +       static STDMETHODIMP
>> +       CreateInstance(CNDAdapter& adapter, VOID** ppMemoryRegion)
>> +       {
>> +               auto_ref<CNDMemoryRegion> mr(new CNDMemoryRegion(adapter));
> 
> +       return ::HeapAlloc(WinVerbs::NetworkDirect::g_hHeap, 0, size);

Not sure what this is doing here...

> 
>> +               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.

For a trivial factory function like the MR, you're probably right.  I was just applying the same standard to all factory functions, just like you used gotos eventhough you could have just returned.

-Fab