-1
\$\begingroup\$

See this method:

HRESULT CChristianLifeMinistryHtmlView::CustomContextMenu(const POINT* ppt, IUnknown* pcmdtReserved)
{
    IOleWindow* oleWnd = nullptr;
    HWND        hwnd = nullptr;
    HMENU       hMainMenu = nullptr;
    HMENU       hPopupMenu = nullptr;
    HRESULT     hr = 0;
    INT         iSelection = 0;

    bool bContinue = true;

    if ((ppt == nullptr) || (pcmdtReserved == nullptr))
        bContinue = false;

    if (bContinue)
    {
        hr = pcmdtReserved->QueryInterface(IID_IOleWindow, reinterpret_cast<void**>(&oleWnd));
        if ((hr != S_OK) || (oleWnd == nullptr))
            bContinue = false;
    }

    if (bContinue)
    {
        hr = oleWnd->GetWindow(&hwnd);
        if ((hr != S_OK) || (hwnd == nullptr))
            bContinue = false;
    }

    if (bContinue)
    {
        hMainMenu = LoadMenu(AfxGetResourceHandle(), MAKEINTRESOURCE(IDR_MENU_HTML_POPUP));
        if (hMainMenu == nullptr)
            bContinue = false;
    }

    if (bContinue)
    {
        hPopupMenu = GetSubMenu(hMainMenu, 0);
        if (hPopupMenu == nullptr)
            bContinue = false;
    }

    if (!bContinue)
    {
        if (hMainMenu != nullptr)
            ::DestroyMenu(hMainMenu);

        return S_OK;
    }

    ::SetMenuItemBitmaps(hPopupMenu, IDM_PAGESETUP, MF_BYCOMMAND,
        (HBITMAP)m_bmpPageSetup, (HBITMAP)m_bmpPageSetup);
    ::SetMenuItemBitmaps(hPopupMenu, IDM_PRINTPREVIEW, MF_BYCOMMAND,
        (HBITMAP)m_bmpPrintPreview, (HBITMAP)m_bmpPrintPreview);
    ::SetMenuItemBitmaps(hPopupMenu, IDM_REFRESH, MF_BYCOMMAND,
        (HBITMAP)m_bmpRefresh, (HBITMAP)m_bmpRefresh);

    // Show shortcut menu
    iSelection = ::TrackPopupMenu(hPopupMenu,
        TPM_LEFTALIGN | TPM_RIGHTBUTTON | TPM_RETURNCMD,
        ppt->x,
        ppt->y,
        0,
        hwnd,
        (RECT*)nullptr);

    // Send selected shortcut menu item command to shell
    if (iSelection != 0)
    {
        if (iSelection == IDM_PRINTPREVIEW)
        {
            ::SendMessage(GetParent()->GetSafeHwnd(), WM_COMMAND, ID_FILE_PRINT_PREVIEW, NULL);
        }
        else if (iSelection == CUSTOM_MENU_EXPORT)
        {
            ::SendMessage(GetParent()->GetSafeHwnd(), WM_COMMAND, ID_FILE_EXPORT, NULL);
        }
        else if (iSelection == IDM_REFRESH)
        {
            ::SendMessage(GetParent()->GetSafeHwnd(), WM_COMMAND, ID_VIEW_REFRESH, NULL);
        }
        else
        {
            ::SendMessage(hwnd, WM_COMMAND, iSelection, NULL);
        }
    }

    return S_OK;
}

Why is it that Visual Studion 2022 Preview 7 (x86 build) is reporting:

  • warning C26430: Symbol pcmdtReserved is not tested for nullness on all paths (f.23).
  • warning C26430: Symbol oleWnd is not tested for nullness on all paths (f.23).

By my use of the bool bContinue flag I have taken into account all pointers and tested them for nullness.

What changes can be made to this function to prevent these warnings?

\$\endgroup\$
5
  • \$\begingroup\$ You could simply disable the warnings, what is it you're really after? \$\endgroup\$
    – Mast
    Oct 27, 2021 at 8:50
  • \$\begingroup\$ @Mast I know I can disable them. What I want to know is why it thinks I have not covered all paths. Is my logic flawed somehow? \$\endgroup\$ Oct 27, 2021 at 8:53
  • 2
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Oct 27, 2021 at 11:29
  • 1
    \$\begingroup\$ The question doesn't really belong on Code Review, it seems to me it would be more appropriate on Stack Overflow, however the answer is good so I'm voting to leave open. \$\endgroup\$
    – pacmaninbw
    Oct 27, 2021 at 13:28
  • 3
    \$\begingroup\$ If you have any questions about the scope of the site, the help center would be a good place to start. If you happen to have additional questions after that, feel free to drop by in chat. \$\endgroup\$
    – Mast
    Oct 27, 2021 at 15:34

1 Answer 1

2
\$\begingroup\$

variable initialization:

Don't declare all the variables at the top of functions. Declare them at the point of first use, and initialize them to meaningful values.


return early on failure:

bool bContinue = true;

if ((ppt == nullptr) || (pcmdtReserved == nullptr))
    bContinue = false;

This could be more succinctly written as:

bool bContinue = (ppt != nullptr) && (pcmdtReserved != nullptr);

which might help the static analysis tool.

However, we can avoid the need for this variable, and the added complexity it introduces by returning early on failure:

if (ppt == nullptr || pcmdtReserved == nullptr)
    return S_OK; // ?!?! see below...

do we really want to ignore errors?

if (!bContinue)
{
    if (hMainMenu != nullptr)
        ::DestroyMenu(hMainMenu);

    return S_OK;
}

Do we really want to return S_OK on failure? Perhaps we should return the relevant failing HRESULT?

The calling code can choose to ignore the failure if it wants to, but if we don't pass the error on it removes any option to do something else, like log the failure.


use assert to avoid extra complexity:

    hr = pcmdtReserved->QueryInterface(IID_IOleWindow, reinterpret_cast<void**>(&oleWnd));
    if ((hr != S_OK) || (oleWnd == nullptr))
        bContinue = false;

MSDN says:

Upon successful return, *ppvObject (the dereferenced address) contains a pointer to the requested interface. If the object doesn't support the interface, the method sets *ppvObject (the dereferenced address) to nullptr.

So we don't need to check oleWnd if hr is S_OK. We could, however, use an assert() to demonstrate that we're relying on that fact.

This means that we can return early with a failing HRESULT, but we don't have to invent a failing HRESULT if oleWnd is nullptr, e.g.:

if (auto hr = pcmdtReserved->QueryInterface(IID_IOleWindow, reinterpret_cast<void**>(&oleWnd)); hr != S_OK) {
    return hr;
}

assert(oldWnd != nullptr);

check return values:

::SetMenuItemBitmaps(hPopupMenu, IDM_PAGESETUP, MF_BYCOMMAND,
    (HBITMAP)m_bmpPageSetup, (HBITMAP)m_bmpPageSetup);
::SetMenuItemBitmaps(hPopupMenu, IDM_PRINTPREVIEW, MF_BYCOMMAND,
    (HBITMAP)m_bmpPrintPreview, (HBITMAP)m_bmpPrintPreview);
::SetMenuItemBitmaps(hPopupMenu, IDM_REFRESH, MF_BYCOMMAND,
    (HBITMAP)m_bmpRefresh, (HBITMAP)m_bmpRefresh);

These can also fail, we might check for that. (If we expect them to never fail, we can assert on the return value. If we want to ignore errors, we can cast the return value to void to explicitly show that we don't care).

        ::SendMessage(GetParent()->GetSafeHwnd(), WM_COMMAND, ID_FILE_PRINT_PREVIEW, NULL);

Again, we might want to check the return value.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your answer. \$\endgroup\$ Oct 27, 2021 at 11:48

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.