2
\$\begingroup\$

In the Windows operating system, some system events are notified via broadcast window messages. DBT_DEVICEARRIVAL and DBT_DEVICEREMOVECOMPLETE are good examples of this. To receive and process these notifications, one need to have a window with an associated window procedure. Given a non-GUI application, this is a burden. So, I'm thinking about encapsulating a simple class for capturing broadcast window messages, and here it comes. The pack comes in one main class CWndMsgReceiver and two helper classes CDummyWndClass and CDummyWnd.

I hope to have my code reviewed, and get some comments and suggestions.

WndMsgReceiver.h

#pragma once
#include <atomic>
#include <thread>
#include <windows.h>

class CDummyWndClass {
public:
  explicit CDummyWndClass(WNDPROC wndproc);
  ~CDummyWndClass();
  operator PCWSTR() const;
private:
  ATOM atom_;
};

class CDummyWnd {
public:
  explicit CDummyWnd(WNDPROC wndproc);
  ~CDummyWnd();
  operator HWND() const;
private:
  CDummyWndClass wndcls_;
  HWND hwnd_;
};

class CWndMsgReceiver {
public:
  explicit CWndMsgReceiver(WNDPROC wndproc);
 ~CWndMsgReceiver();
  operator HWND() const;  // to use with RegisterDeviceNotification
private:
  static void MsgPump(WNDPROC wndproc, std::atomic<HWND>& hwnd);
  std::atomic<HWND> hwnd_;
  std::thread thrd_;
};

WndMsgReceiver.cpp

#include "WndMsgReceiver.h"
#include <cstring>
#include <functional>
#include <utility>

CDummyWndClass::CDummyWndClass(WNDPROC wndproc)
    : atom_(0) {
  HMODULE hmodule = 0;
  GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS |
                     GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT,
                     (PCWSTR)wndproc, &hmodule);
  WNDCLASSW wndcls = {0, wndproc, 0, 0, (HINSTANCE)hmodule, 0, 0, 0, nullptr, 
                      L"{F9F40031-F77E-4A5C-8A6C-30296B823C22}"};
  atom_ = RegisterClassW(&wndcls);
}

CDummyWndClass::~CDummyWndClass() {
  UnregisterClassW((PCWSTR)*this, 0);
}

CDummyWndClass::operator PCWSTR() const {
  PCWSTR p = nullptr;
  std::memcpy(&p, &atom_, sizeof(atom_));
  return p;
}

////////////////////////////////////////////////////////////////////////////////

CDummyWnd::CDummyWnd(WNDPROC wndproc)
    : wndcls_(wndproc)
    , hwnd_(CreateWindowW((PCWSTR)wndcls_, nullptr, 0, 0, 
                          0, 0, 0, 0, 0, 0, nullptr)) {
}

CDummyWnd::~CDummyWnd() {
  DestroyWindow(hwnd_);
}

CDummyWnd::operator HWND() const {
  return hwnd_;
}

////////////////////////////////////////////////////////////////////////////////

CWndMsgReceiver::CWndMsgReceiver(WNDPROC wndproc)
    : hwnd_{}
    , thrd_(std::thread(MsgPump, wndproc, std::ref(hwnd_))) {
  do Sleep(0); while (!(HWND)hwnd_);
}

CWndMsgReceiver::~CWndMsgReceiver() {
  PostThreadMessageW(GetThreadId(thrd_.native_handle()), WM_QUIT, 0, 0);
  thrd_.join();
}

CWndMsgReceiver::operator HWND() const {
  return hwnd_;
}

void CWndMsgReceiver::MsgPump(WNDPROC wndproc, std::atomic<HWND>& hwnd) {
  CDummyWnd wnd(wndproc);
  hwnd = wnd;
  MSG msg = {};
  while (GetMessageW(&msg, 0, 0, 0)) {
    TranslateMessage(&msg);
    DispatchMessageW(&msg);
  }
}
\$\endgroup\$
1
\$\begingroup\$

I haven't worked with WinAPI in years, so I can't help you much with that, but I do have a few concerns regarding style and structure of your code:

  • What's the meaning of this GUID? Does it have any, or is it just randomly generated?

    WNDCLASSW wndcls = {0, wndproc, 0, 0, (HINSTANCE)hmodule, 0, 0, 0, nullptr, 
                  L"{F9F40031-F77E-4A5C-8A6C-30296B823C22}"};
    

    A comment explaining why this specific value would be nice. Or better, make it a named constant with a self-explanatory name.

  • You have several instances of C-style casts in the code. Prefer using the C++ cast operators instead. They will give you warnings and diagnostics for potentially dangerous casts.

  • This looks very obscure:

    CDummyWndClass::operator PCWSTR() const {
      PCWSTR p = nullptr;
      std::memcpy(&p, &atom_, sizeof(atom_));
      return p;
    }
    

    On my first look, I thought that you were trying to memcpy into a null pointer. The & on p is not easily spotted without a careful look. If you really must do this, then I think a reinterpret_cast would be more clear about your intentions. I did not test this, but something along the lines of the following should do:

    CDummyWndClass::operator PCWSTR() const {
        const std::uintptr_t intVal = static_cast<std::uintptr_t>(atom_);
        PCWSTR ptrVal = reinterpret_cast<PCWSTR>(intVal);
        return ptrVal;
    }
    
  • It would probably look clearer if you'd lay out that loop inside CWndMsgReceiver construtor is a more spaced way, to give it emphasis. Default { } is always good too:

      do Sleep(0); while (!(HWND)hwnd_);
    
     do {
         Sleep(0); 
     } while (!(HWND)hwnd_);
    

    You should also comment that this is not an infinite loop, but that it waits on an independent thread to set the variable.

  • This might be your personal preference, I for one I'm not a big fan of cast operator overloads. I've seen instances when they can produce some pretty obscure code. IMHO, I think your cast operators would look much nicer as plain Get*() methods.

  • You don't seem to use the wacky Microsoft prefixes on variable names, though you do use the C prefix for class names. Again, this is a subjective point, but I think your class names would be more readable without it.

\$\endgroup\$
1
  • \$\begingroup\$ Your suggestions are reasonable, and I'm going to improve my code accordingly. The GUID is there just to be unique to avoid name conflicts (the window class is never referenced by name, but atom). \$\endgroup\$ – Lingxi Apr 21 '15 at 1:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.