[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: AW: Conceptual Patch - Handle wrapper class

From: Stefan Küng <tortoisesvn_at_gmail.com>
Date: Tue, 15 Mar 2011 22:09:41 +0100

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

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.


  oo  // \\      "De Chelonian Mobile"
 (_,\/ \_/ \     TortoiseSVN
   \ \_/_\_/>    The coolest Interface to (Sub)Version Control
   /_/   \_\     http://tortoisesvn.net
To unsubscribe from this discussion, e-mail: [dev-unsubscribe_at_tortoisesvn.tigris.org].
Received on 2011-03-15 22:10:06 CET

This is an archived mail posted to the TortoiseSVN Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.