Opened 4 months ago

Closed 3 months ago

Last modified 3 months ago

#16233 closed enhancement (fixed)

Add RAII wrapper for Win32 API event object

Reported by: troelsk Owned by: VZ
Priority: low Milestone:
Component: wxMSW Version: dev-latest
Keywords: work-needed Cc:
Blocked By: Blocking:
Patch: yes

Description

Patch:
Simple CreateEvent()-wrapper, declared in msw/private.h.

Attachments (4)

wxWinEvent.patch download (7.9 KB) - added by troelsk 4 months ago.
wxWinAPI-Event.patch download (9.7 KB) - added by troelsk 4 months ago.
using namespace etc
wxWinAPI-Event(1).patch download (9.6 KB) - added by troelsk 4 months ago.
using namespace etc
wxWinAPI-Event(2).patch download (10.7 KB) - added by troelsk 4 months ago.
Added lock() method

Download all attachments as: .zip

Change History (13)

Changed 4 months ago by troelsk

comment:1 Changed 4 months ago by vadz

  • Keywords work-needed added
  • Priority changed from normal to low
  • Status changed from new to confirmed
  • Summary changed from Windows event class to Add RAII wrapper for Win32 API event object

The idea is good and I'm all for having more RAII wrappers for Windows classes, but I have several remarks concerning the implementation:

  1. I think this should be defined in a new include/wx/msw/private/event.h file.
  2. It should also be called wxWinAPI::Event or something like this, i.e. I'd like to use a namespace instead of the "wx" prefix to distinguish this class from the public classes and I'd like to avoid "wxWinEvent" as this is too close to "wxEvent".
  3. The new class should probably derive from the existing AutoHandle.
  4. I'd strongly prefer to avoid using bool params and have enum Kind { Manual, Automatic } and so on instead.
  5. It should probably be implemented entirely inline as all of its methods are almost trivial.
  6. OTOH it would be nice to have some minimal error reporting, i.e. wxLogLastError() in case of API call errors.
  7. The class should be non-copyable.

TIA!

Changed 4 months ago by troelsk

using namespace etc

comment:2 Changed 4 months ago by troelsk

Thanks for feedback! Added rewritten patch. Had to modify AutoHANDLE to handle NULL's (AutoHANDLE seems to be not used at all currently).
Author is mostly you, I only did the typing.

Last edited 4 months ago by troelsk (previous) (diff)

Changed 4 months ago by troelsk

using namespace etc

Changed 4 months ago by troelsk

Added lock() method

comment:3 Changed 4 months ago by troelsk

One more wrapper. This can be used in 2 places, in the wx code. Is good or?

class MultiLock : public wxVector<WXHANDLE>
{
public:
    wxUint32 Lock();
};

inline wxUint32 wxWinAPI::MultiLock::Lock()
{
   return ::WaitForMultipleObjects((DWORD)size(), &at(0), false, INFINITE);
}

comment:4 Changed 3 months ago by vadz

I don't think Lock() should be in this class, it's not at all specific to the events. And I think MultiLock makes the code less, not more, readable, so I won't apply this for now. I am applying the rest however (with some changes), thanks!

comment:5 Changed 3 months ago by VZ

In 76653:

Allow wxMSW AutoHANDLE helper to be used with more kinds of handles.

Make AutoHANDLE a template to allow specifying the invalid handle value, which
is inconsistent across Win32 API and is INVALID_HANDLE_VALUE for most kinds of
handles but 0 for some others, e.g. event object handles.

See #16233.

comment:6 Changed 3 months ago by VZ

In 76654:

Add a simple wrapper for Windows event handle object.

This will be used instead of raw events in wxMSW code in the upcoming commit.

See #16233.

comment:7 Changed 3 months ago by VZ

  • Owner set to VZ
  • Resolution set to fixed
  • Status changed from confirmed to closed

In 76655:

Use wxWinAPI::Event wrapper class instead of raw Windows event handles.

This makes the code slightly shorter and, more importantly, more readable and
safer.

Closes #16233.

comment:8 Changed 3 months ago by VZ

In 76658:

Explicitly convert int to HANDLE to fix 64 bit wxMSW build.

This fixes g++ compilation problem in 64 bit mode after the changes of r76653,
it complained about comparing pointer (HANDLE) with an integer. It might also
fix compilation with icc.

See #16233.

comment:9 Changed 3 months ago by VZ

In 76659:

Compilation fix for wxMSW 64 bit build with icc.

This compiler doesn't want to narrow INVALID_HANDLE_VALUE to int, so use
wxUIntPtr instead.

See #16233.

Note: See TracTickets for help on using tickets.