-5

The general rule of thumb is not to call a virtual function from a constructor because it can lead to unpredictable behavior. So why does it work sometimes?

I recently wrote a couple of base classes with pure virtual functions, and accidentally included an indirect call to those functions in the constructor. I realized my mistake and corrected it, but one of them worked while the other did not.

Here's the definition of the class that worked:

template <typename TWindow>
class dialog_base
{
    static INT_PTR CALLBACK dlg_proc_internal
        (HWND,
         UINT,
         WPARAM,
         LPARAM);
protected:
    dialog_base
        (const LPCWSTR templateName,
         const HWND parent)
        {
        CREATESTRUCT create;
        create.lpCreateParams = this;
        m_hwnd = CreateDialogParam(
            hinstance_, templateName, parent, dlg_proc_internal,
            reinterpret_cast<LPARAM>(&create));
        }

    HWND m_hwnd;
    virtual INT_PTR CALLBACK dlg_proc
        (UINT,
         WPARAM,
         LPARAM) = 0;
public:
    virtual ~dialog_base()
        {
        DestroyWindow(m_hwnd);
        }

    HWND GetHandle() const;
    void show() const;
};

In this class, the DialogBoxParam function calls dlg_proc_internal, passing the WM_NCCREATE message:

template <typename TWindow>
INT_PTR dialog_base<TWindow>::dlg_proc_internal
    (HWND hWnd,
     UINT msg,
     WPARAM wParam,
     LPARAM lParam)
    {
    dialog_base<TWindow>* pThis;
    if (msg == WM_NCCREATE)
        {
        pThis = static_cast<dialog_base<TWindow>*>(reinterpret_cast<
            CREATESTRUCT*>(lParam)->lpCreateParams);
        SetLastError(0);
        if (!SetWindowLongPtr(
            hWnd, GWLP_USERDATA,
            reinterpret_cast<LONG_PTR>(pThis)) && GetLastError() != 0)
            return 0;
        }
    else
        {
        pThis = reinterpret_cast<dialog_base<TWindow>*>(
            GetWindowLongPtr(hWnd, GWLP_USERDATA));
        }
    return pThis
               ? pThis->dlg_proc(msg, wParam, lParam)
               : DefWindowProc(hWnd, msg, wParam, lParam);
    }

This function retrieves the pointer passed as the last argument to CreateDialogParam and stores it in the window so that it can be retrieved again in later calls to the function. It then mistakenly calls the pure virtual function dlg_proc instead of returning -- and appears to work just fine through the constructor of a child class.

I created a different class which was almost identical, except that it called CreateWindowEx instead of CreateDialogParam. The pointer argument was passed in much the same way, and used to call the pure virtual function. This time, it failed as one might expect. So what's the difference between the two situations?

EDIT:

Perhaps I should clarify. I'm not asking "Why can't I call virtual members from a constructor?". I'm asking about why the process of resolving virtual members before the object is constructed can sometimes create situations in which an error does not occur, and the correct function is called.

mousebyte
  • 61
  • 7
  • 5
    "because it can lead to unpredictable behavior. So why does it work sometimes". Because it is undefined!!!! behavior! – Klaus Aug 06 '18 at 07:34
  • It works by accident because you never actually call dlg_proc(). You have a bug in the SetWindowLongPtr() usage, you assumed it failed but it didn't. It simply returns 0 because that was the previous GWLP_USERDATA value. – Hans Passant Aug 06 '18 at 07:43
  • 1
    I could probably explain this from a Win32 and C++ perspective. But I would need to see a minimum, complete verifiable example. That is, something I could cut and paste and debug directly. Can you share a complete working example? https://stackoverflow.com/help/mcve – selbie Aug 06 '18 at 07:49
  • @HansPassant I have confirmed in debug that dlg_proc() is in fact called and control passes to the child class member. – mousebyte Aug 06 '18 at 08:15
  • @mousebyte Related: [Create window class for Dialog](https://stackoverflow.com/questions/21658656/). You are not passing your `this` pointer into your message procedure correctly. – Remy Lebeau Aug 06 '18 at 17:04

2 Answers2

3

Calling a virtual function from a constructor has perfectly predictable behavior in C++, just like it has perfectly predictable behavior in .Net and Java. However, it's not the same behavior.

In C++, virtual functions dispatch on the type of the object at the moment of calling. Some other languages will use the intended type of object. Both are viable choices, both have risks, but since this is a C++ question I'll focus on the C++ risk.

In C++, virtual functions can be pure virtual. dlg_proc in the question is such a pure virtual function. These are declared in the base class, but not (necessarily) defined there. Trying to call a function that you did not define is Undefined Behavior. Compilers are entirely free to do whatever they like.

One possible implementation is that a compiler just calls a random other function. This could be remove(filename). It might also be the override from a derived class. But there are a million other possible results, including crashes and hangs. So we don't try to predict what happens, and just say: don't call pure virtual functions from ctors.

Footnote: You can actually provide a body for a pure virtual function; the language allows it.

MSalters
  • 173,980
  • 10
  • 155
  • 350
1

CreateDialog...() (and DialogBox...()) does not pass its dwInitParam parameter value to your message procedure via WM_(NC)CREATE. It is passed via WM_INITDIALOG instead, which you are not handling. Only CreateWindow/Ex() passes its lpParam parameter values to the message procedure via WM_(NC)CREATE. This is documented behavior!

But more importantly, you are manually passing a CREATESTRUCT to CreateDialogParam(). That is not necessary, especially since you are not handling that extra CREATESTRUCT in your WM_NCCREATE handler. When the system issues WM_(NC)CREATE to a window, the lParam passed to CreateWindow/Ex() gets wrapped in a system-provided CREATESTRUCT. So, even if CreateDialogParam() were to pass its dwInitParam as the lParam to CreateWindowEx() (which is not documented behavior, BTW), you still wouldn't be obtaining your dialog_base* pointer correctly inside your message procedure, as you are not handling that 2 separate CREATESTRUCTs could be present. So, your code has undefined behavior when using the pThis pointer for any reason, since you are not passing that pointer value into your message procedure correctly.

You need to pass your this pointer directly to CreateDialogParam() without wrapping it, and you need to handle WM_INITDIALOG instead of WM_NCCREATE. Then your virtual method should behave as expected (ie, it will not dispatch to a derived class, since WM_INITDIALOG is being handled within the context of the base class constructor).

Also, DO NOT call DefWindowProc() in your message procedure (or derived overrides) when using CreateDialog...() (or DialogBox...()). This is specifically stated in the DialogProc documentation:

Although the dialog box procedure is similar to a window procedure, it must not call the DefWindowProc function to process unwanted messages. Unwanted messages are processed internally by the dialog box window procedure.

Try this instead:

template <typename TWindow>
class dialog_base
{
    static INT_PTR CALLBACK dlg_proc_internal(HWND, UINT, WPARAM, LPARAM);

protected:
    dialog_base(LPCWSTR templateName, HWND parent)
    {
        m_hwnd = CreateDialogParamW(hinstance_, templateName, parent, dlg_proc_internal, reinterpret_cast<LPARAM>(this));
    }

    HWND m_hwnd;
    virtual INT_PTR CALLBACK dlg_proc(UINT, WPARAM, LPARAM) = 0;

public:
    virtual ~dialog_base()
    {
        DestroyWindow(m_hwnd);
    }

    HWND GetHandle() const;
    void show() const;
};

template <typename TWindow>
INT_PTR dialog_base<TWindow>::dlg_proc_internal (HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam)
{
    dialog_base<TWindow>* pThis;

    if (msg == WM_INITDIALOG)
    {
        pThis = reinterpret_cast<dialog_base<TWindow>*>(lParam);

        // you CANT cancel dialog creation here when
        // using CreateDialog...(), only when using
        // DialogBox...()!  So, no point in doing any
        // error checking on SetWindowLongPtr()...
        SetWindowLongPtr(hWnd, GWLP_USERDATA, reinterpret_cast<LONG_PTR>(pThis));

        // no point in trying to call pThis->dlg_proc()
        // here since it won't be dispatched to derived
        // classes anyway...
        return TRUE; // or FALSE, depending on your needs...
    }

    pThis = reinterpret_cast<dialog_base<TWindow>*>(GetWindowLongPtr(hWnd, GWLP_USERDATA));
    if (!pThis) return FALSE;
    return pThis->dlg_proc(msg, wParam, lParam);
}
Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770