WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
55334
Add a handler class for Win32 HANDLE
https://bugs.webkit.org/show_bug.cgi?id=55334
Summary
Add a handler class for Win32 HANDLE
Patrick R. Gansterer
Reported
2011-02-27 15:23:31 PST
see patch
Attachments
Patch
(3.15 KB, patch)
2011-02-27 15:28 PST
,
Patrick R. Gansterer
aroben
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Patrick R. Gansterer
Comment 1
2011-02-27 15:28:39 PST
Created
attachment 83998
[details]
Patch
Adam Roben (:aroben)
Comment 2
2011-02-28 05:10:46 PST
Comment on
attachment 83998
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=83998&action=review
r=me, but I'm not marking it cq+ because I think you should make a few changes.
> Source/WebCore/platform/win/Win32Handle.h:36 > + explicit Win32Handle(HANDLE handle) : m_handle(handle) { }
It would be nice if we could be more explicit that this function adopts the passed-in HANDLE.
> Source/WebCore/platform/win/Win32Handle.h:44 > + CloseHandle(m_handle);
Is it OK to call CloseHandle(0)?
> Source/WebCore/platform/win/Win32Handle.h:50 > + operator HANDLE() const { return m_handle; }
I don't think we need this. We can just use .get() instead. That would match our WTF smart pointers better.
> Source/WebCore/platform/win/Win32Handle.h:59 > + Win32Handle& operator=(Win32Handle& other) { swap(other); }
This is surprising behavior. I think it would make more sense just to make Win32Handle be non-copyable.
Patrick R. Gansterer
Comment 3
2011-02-28 06:13:20 PST
Comment on
attachment 83998
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=83998&action=review
>> Source/WebCore/platform/win/Win32Handle.h:36 >> + explicit Win32Handle(HANDLE handle) : m_handle(handle) { } > > It would be nice if we could be more explicit that this function adopts the passed-in HANDLE.
Should I create a adoptWin32Handle ?!? Then this class can't be non-copyable or we need PassWin32Handle too?
>> Source/WebCore/platform/win/Win32Handle.h:44 >> + CloseHandle(m_handle); > > Is it OK to call CloseHandle(0)?
When will m_handle become 0?
>> Source/WebCore/platform/win/Win32Handle.h:50 >> + operator HANDLE() const { return m_handle; } > > I don't think we need this. We can just use .get() instead. That would match our WTF smart pointers better.
I don't like to use .get() all the time, but it makes sense to match the OwnPtr interface.
Adam Roben (:aroben)
Comment 4
2011-02-28 07:11:16 PST
Comment on
attachment 83998
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=83998&action=review
>>> Source/WebCore/platform/win/Win32Handle.h:36 >>> + explicit Win32Handle(HANDLE handle) : m_handle(handle) { } >> >> It would be nice if we could be more explicit that this function adopts the passed-in HANDLE. > > Should I create a adoptWin32Handle ?!? Then this class can't be non-copyable or we need PassWin32Handle too?
Yeah, I'm not sure what the best idiom here is. I think what you have now is fine.
>>> Source/WebCore/platform/win/Win32Handle.h:44 >>> + CloseHandle(m_handle); >> >> Is it OK to call CloseHandle(0)? > > When will m_handle become 0?
It could be 0 if someone passed 0 to the constructor.
Adam Roben (:aroben)
Comment 5
2011-03-01 14:13:33 PST
The version of this patch that landed in
r80034
has neither a copy constructor nor the WTF_MAKE_NONCOPYABLE macro. This means the compiler will generate a default copy constructor, which will do the wrong thing.
Patrick R. Gansterer
Comment 6
2011-03-01 14:14:42 PST
(In reply to
comment #5
)
> The version of this patch that landed in
r80034
has neither a copy constructor nor the WTF_MAKE_NONCOPYABLE macro. This means the compiler will generate a default copy constructor, which will do the wrong thing.
Argh! Missed to add the macro :-(
Patrick R. Gansterer
Comment 7
2011-03-01 14:37:48 PST
(In reply to
comment #5
)
> The version of this patch that landed in
r80034
has neither a copy constructor nor the WTF_MAKE_NONCOPYABLE macro. This means the compiler will generate a default copy constructor, which will do the wrong thing.
Sorry again, committed
r80042
: <
http://trac.webkit.org/changeset/80042
> (In reply to
comment #4
)
> >> Is it OK to call CloseHandle(0)? > > > > When will m_handle become 0? > > It could be 0 if someone passed 0 to the constructor.
IMHO passing 0 is not right correct, but there's no problem to call CloseHandle(0). It will set GetLastError() to sth like "called with invalid handle" only.
Adam Roben (:aroben)
Comment 8
2011-03-01 14:38:35 PST
(In reply to
comment #7
)
> (In reply to
comment #4
) > > >> Is it OK to call CloseHandle(0)? > > > > > > When will m_handle become 0? > > > > It could be 0 if someone passed 0 to the constructor. > > IMHO passing 0 is not right correct, but there's no problem to call CloseHandle(0). It will set GetLastError() to sth like "called with invalid handle" only.
If we don't want to allow 0, we could add an assertion.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug