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
_______________________________________________
ofw mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
