On Tue, Mar 15, 2011 at 21:54, Oto BREZINA <otik_at_printflow.eu> wrote:
> This also shifts Invalid handle value to NULL. What make me think if
> patch r20974 was made in right way.
>
> For m_hPipe is errorous INVALID_HANDLE_VALUE, while for m_hEvent it is NULL.
>
> So ClosePipe should looks like:
>
> void CRemoteCacheLink::ClosePipe()
> {
> AutoLocker lock(m_critSec);
>
> if(m_hPipe != INVALID_HANDLE_VALUE)
> {
> CloseHandle(m_hPipe);
> m_hPipe = INVALID_HANDLE_VALUE;
> }
> if(m_hEvent != NULL)
> {
> CloseHandle(m_hEvent);
> m_hEvent = NULL;
> }
> }
>
> Is this easy readable and maintainable ?
r20974 is correct. To avoid dealing with NULL and
INVALID_HANDLE_VALUE, the m_hEvent is manually set to
INVALID_HANDLE_VALUE in case it is set to NULL by the API call.
It's readable, and as maintainable as the API allows.
>> So IMHO while the idea of a wrapper class is good, it is dangerous.
>> I'd like to keep the code as is, using the handles for each API
>> separately according to its documentation.
> I would say not using it is dangerous, using class built without
> understanding is dangerous even more. That's one of most important
> reasons I send as the conceptual patch, not just a patch. Something to
> check.
>
> It's your code and you are still main developer - you would be affected
> mostly; I just tried to share my thoughts...
Using a wrapper class is dangerous here, because once we start using
it, people tend to get lazy (at least I do) and forget to check and
make sure the class can really be used. For most situations, it would
work great. But then comes the situation where the API actually allows
INVALID_HANDLE_VALUE as a pseudo handle value and since we're so used
to using the wrapper, we simply don't check and then it will fail.
Definitely not while we're debugging it but later on some users
machine...
That's why I don't like to use wrapper classes if there's no way to
make them work in all situations, or break *reliably* in situations
where it must not be used.
And I think that's exactly the reason why the existing CHandle class
only supports NULL as the invalid value and not INVALID_HANDLE_VALUE.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
------------------------------------------------------
http://tortoisesvn.tigris.org/ds/viewMessage.do?dsForumId=757&dsMessageId=2711758
To unsubscribe from this discussion, e-mail: [dev-unsubscribe_at_tortoisesvn.tigris.org].
Received on 2011-03-15 22:10:06 CET