MenuIcon problem with latest version of NPP

pnedev

Hi again,

If the code below is the original one that is not working as expected I might have an explanation why it is not behaving well.

void Accelerator::updateMenuItemByCommand(CommandShortcut csc)
{
    MENUITEMINFO mii;
    mii.cbSize = sizeof(MENUITEMINFO);
    mii.fMask = MIIM_STRING;
    mii.dwTypeData = const_cast<LPTSTR>(csc.toMenuItemString().c_str());
    ::SetMenuItemInfo(_hAccelMenu, csc.getID(), FALSE, &mii);
}

The problem is that when SetMenuItemInfo is called the mii.dwTypeData is not valid.
That’s because mii.dwTypeData is a pointer to a TCHAR array - it is not the actual array and something needs to hold the actual data. But const_cast<LPTSTR>(csc.toMenuItemString().c_str()) is not holding anything because toMenuItemString() returns temporal std::basic_string<TCHAR> object that is not saved in our scope and this temporal string ceases to exist the moment its pointer is stored in mii.dwTypeData. At the moment SetMenuItemInfo is called the mii.dwTypeData is pointing to a free memory that is on the stack and is overwritten the moment a new automatic variable is created.

That’s why Claudia’s solution with local variable that is holding the new string is working - mii.dwTypeData is pointing to local array that is still valid when SetMenuItemInfo is called.

BR,
Pavel

Yaron

Hello Pavel,

Thank you for the explanation. I appreciate it.

There’s still a “missing link”:
What’s the difference between

::ModifyMenu(_hAccelMenu, cmdID, cmdFlags, cmdID, csc.toMenuItemString().c_str());

and

LPCTSTR test =  csc.toMenuItemString().c_str();
::ModifyMenu(_hAccelMenu, cmdID, cmdFlags, cmdID, test);

?

Best regards.

pnedev

Hello Yaron,

The problem is the same.

In

LPCTSTR test =  csc.toMenuItemString().c_str();
::ModifyMenu(_hAccelMenu, cmdID, cmdFlags, cmdID, test);

test is a pointer to const TCHAR that is pointing to the data of a temporary std::basic_string<TCHAR> object returned by toMenuItemString(). The problem is that the string object (the one actually holding the data) is temporary - it is not saved by the assignement to test and it is destroyed immediately after the assignment.

That code is practically equivalent to:

int returnInteger()
{
    int ret = 5;
    return ret;
}

const int* pInt = returnInteger();

Here pInt is pointing to nonexistent int object.

Now

::ModifyMenu(_hAccelMenu, cmdID, cmdFlags, cmdID, csc.toMenuItemString().c_str());

is a function call which takes a pointer to temporary object. I’m not sure how this will behave and if it will behave consistently if the code is compiled with different compilers but that might work. That’s because here the temporary might still be alive even though it is not stored anywhere - it might still live on the stack. For sure the temporary ceases to exist on the next line (as is the case with the ‘in-between’ assignment).
As an illustration (continued from the above one) it is equivalent to

void useIntPointer(const int* pInt)
{
    ...
}

useIntPointer(returnInteger());

To be perfectly safe there’s nothing wrong with doing that:

std::basic_string<TCHAR> menuItem =  csc.toMenuItemString();
::ModifyMenu(_hAccelMenu, cmdID, cmdFlags, cmdID, menuItem.c_str());

Now the menuItem variable is actually holding the whole string data (it is not just a pointer) and it will be valid in our current scope.
The new compilers might not even make a copy of the string (there might be no assignment penalty) as they might simply apply move operation and directly reuse the string data returned by toMenuItemString().

BR,
Pavel

Yaron

Hello Pavel,

Thank you for the detailed and well built explanation.

The issue is clear.
With your permission, I’ll try to slightly sharpen it.

int * returnIntPointer()
{
    int ret = 5;
    return &ret;
}

void useIntPointer(const int* pInt)
{
    ...
}

useIntPointer(returnIntPointer());

ret is stored and valid for the duration of useIntPointer().
It should lose its validity only when we reach the end of useIntPointer(returnIntPointer());; i.e. when useIntPointer() ends.

Is that correct?

Best regards.

pnedev

Hello Yaron,

You are welcome.

No, the assumption is not correct.

ret is destroyed (stops existing) the moment we exit returnIntPointer():
ret is a local variable of returnIntPointer() after all.

If you return it by value (as in my example) a temporary copy of ret is returned where the call to returnInteger() is. This temporary exist only in that place and if it is not immediately copied then it is lost.

BR

Yaron

Hello Pavel,

ret is a local variable of returnIntPointer() after all.

I understand that.
My idea was that returnIntPointer() is not destroyed until the following ; is reached.
IOW: since we use useIntPointer(returnIntPointer());, it’s valid until we return to );.

Otherwise, is a new temporary copy of ret created?
And what’s the exact definition? You can copy it just at the very beginning of the function? Isn’t it a bit far fetched?

Another option is that you assume there’s not enough time to destroy ret because of the events proximity.
But that doesn’t make sense either.

I suppose this question can be tested.

Thanks again. I appreciate your contribution to MenuIcons as well. :)
BR

pnedev

Hello Yaron,

My idea was that returnIntPointer() is not destroyed until the following ; is reached.
IOW: since we use useIntPointer(returnIntPointer());, it’s valid until we return to );

That’s true for returnInteger() not for returnIntPointer() and returnIntPointer() is a function - it is not destroyed. The value (variable) it returns is not destroyed until the next ;.

Let’s look at returnIntPointer()'s code:

int * returnIntPointer()
{
    int ret = 5;
    return &ret;
}

When we enter the function, ret is created on the stack, the stack pointer is moved (let’s say incremented for clarity).
Now we return ret’s address and exit the function. When we exit, ret is freed -> the stack pointer is decremented.
Now the returned pointer points to free stack memory that will hold the old ret’s value until another local (automatic) variable is created and that usually happens when you enter another function. In our case this function is useIntPointer(). So in useIntPointer() our pointer points to invalid data.

When you return ret by value (as in returnInteger()) the returned value is either in processor register or on the stack before all returnInteger() local variables are put there and later - destroyed. IOW there is no copy performed - it is simply a compiler matter.

Your assumption is correct to the point that whatever variable a function returns (temporary) lives until the next ; where the function is called.
In returnIntPointer() the returned variable is int* and it lives until next ; after the call but it points to what? Some free memory that is overwritten perhaps and assigned to another variable.
In returnInteger() the returned variable is int and it’s just what you need (it is not extra-copied on return, don’t worry, it’s just created by the compiler in the appropriate way). You can assign it to another variable to store it or directly give it to another function (copying it or using it’s address - doesn’t matter) - it is valid until the next ; after the call.

Hope that clarifies.

BR

Yaron

Hello Pavel,

The issue is clearer now.
Some of the terms you’ve used are new to me. I’ll have to expand my basic knowledge. :)

Thank you for the interesting explanation. As always, I appreciate your patience.

BR

Claudia Frank

Hello Yaron and Pavel,

coming back from a short trip it is nice to see that you got it sorted out.

Pavel, thank you very much for your insights - very much appreciated.
Your explanation gives me the feeling that I’ve done another step in
understanding this pointer stuff, although it seems to be more like
an variable lifetime issue. :-)

Yaron, thanks for asking the questions I would have asked also. :-)

Cheers
Claudia

Yaron

Hello Claudia,

Welcome back. I hope you had a good time.
The forum is not what it is when you’re away. :)

Thanks again for your kind help.

Best regards.