#546 THdrItem constructors do not initialize all members

[Ognyan Chernokozhev]

The default constructor of THdrItem does not set the struct members to zero.

This means that the code

THdrItem hdrItem;
hdrItem.SetText(_T("THdrItem 1"));
pColumnHeader->Add(hdrItem);

results in undefined behavior, either not displaying the column headers, or displaying them in a strange way.
The specific issue is that the fmt member is not set, and thus contains a random value, to which the SetText call tries to OR with HDF_STRING.

Other members like hbm, lParam and cxy should also be initialized to zeroes.

[Vidar Hasfjord]

Hi Ognyan, good catch!

Note that your fix [r6286] still doesn't initialise all the members of the base struct HDITEM. Rather than use assignments, we can simply initialise all the members by initialising the base struct in the constructor's member initialiser list:

//
/// Constructs an 'empty' THdrItem with the specified 'msk' enabled.
/// This flavour of the constructor is mainly used to construct an object
/// which is used to retrieve information about an existing item.
//
THdrItem::THdrItem(uint msk)
  : HDITEM{msk}
{}

This is guaranteed to initialise all members of the HDITEM structure (see aggregate initialisation). We can do likewise for the other constructors. I have hence broadened the title and scope of this ticket, and the broader fix has been applied in [r6287].

Edit: The fix has now been merged into Owlet [r6288] and 7.0 [r6289].

[Ognyan Chernokozhev]

Rather than use assignments, we can simply initialise all the members by initialising the base struct in the constructor's member initialiser list

Can this be applied in 6.44 as well? Or should we use ZeroMemory instead for backward C++ compatibility?

[Vidar Hasfjord]

@jogybl wrote:

Can this be applied in 6.44 as well?

No, not without adaptations to C++98. While aggregate initialisation works in C++98 as well, it is unfortunately not available in the constructor's member initialiser list (the ability to use aggregate initialisation in the member initialiser list was introduced in C++11). Instead, we can call the default constructor (which will set the members to zero using value-initialisation) and then use assignment to set specific fields to given values.

THdrItem::THdrItem(uint msk)
  : HDITEM()
{
  mask = msk;
}

Here is a sanity check at Compiler Explorer: https://godbolt.org/z/KafT9qdK8

By the way, should we release urgent updates of 6.44 and 7.0 with this fix? Or should we wait for Luigi to complete VCL support and include that feature in the same releases? If the former, I can merge the fix to 6.44 and prepare updates for immediate release, if you want. Although, you are welcome to take the role as release manager, if you want to. It would be cool to see a Jogy release again!

Full step-by-step guide is documented under [Contributing]:

https://sourceforge.net/p/owlnext/wiki/Contributing/#preparing-and-releasing-a-new-version

[Ognyan Chernokozhev]

By the way, should we release urgent updates of 6.44 and 7.0 with this fix?

No, I don't think it is urgent. There is a very easy workaround - when using the default constructor, manually initialize the fmt and other members to zeroes before calling SetText, and all works fine.

[Vidar Hasfjord]

@jogybl wrote:

No, I don't think it is urgent.

OK, then I will not merge the fix to 6.44 yet, as to keep the release branch clean. We will merge in preparation for release, whenever that may be. Unless you see any problems with our fix, then you can set the status of this ticket to "pending".

There is a very easy workaround - when using the default constructor

Yes. And just to add to your workaround — another workaround is to not use the default constructor, but use the dedicated constructor instead. As documented, the default constructor is meant for retrieving information, not setting it.

THdrItem hdrItem(_T("THdrItem 1"));
pColumnHeader->Add(hdrItem);

Note that, as I pointed out before, there will be uninitialised members in this call, although the mask should exclude them, so they should have no effect.

[Ognyan Chernokozhev]

The problem is that the constructor that accepts the header text, also sets the column width to DefStringItemSize.

And in my specific use case, I need to change just the column header text without changing its width.

[Vidar Hasfjord]

@jogybl wrote:

I need to change just the column header text without changing its width.

In that case you can do this:

THdrItem hdrItem(_T("THdrItem 1"));
hdrItem.mask &= ~HDI_WIDTH; // Ignore width.
// ...

[Ognyan Chernokozhev]

Yeah, that would work, but it is non-intuitive and relies on knowledge of the internals of the THdrItem constructor.

[Vidar Hasfjord]

@jogybl wrote:

Yeah, that would work, but [bit-fiddling] is non-intuitive

Agree. Perhaps a better option is to set the flags you want, rather than turn off unwanted flags.

THdrItem hdrItem(_T("THdrItem 1"));
hdrItem.SetMask(HDI_TEXT | HDI_FORMAT); // Only change text, not width.
// ...

[Vidar Hasfjord]

The fix for this issue was merged into 6.44 in [r6349] as discussed above.