[PATCH v4 5/7] comctl32/listbox: Use a helper to retrieve item string by index

[Huw Davies]

On Wed, Feb 13, 2019 at 12:45:52PM +0200, Gabriel Ivăncescu wrote:
> On 2/13/19 11:50 AM, Huw Davies wrote:
> > On Tue, Feb 12, 2019 at 03:47:01PM +0200, Gabriel Ivăncescu wrote:
> > > Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
> > > ---
> > > 
> > > v4: Dumb mistake with get_item_string, sorry for noise.
> > > 
> > > I've left FindStringPos alone because I'm not 100% sure if a listbox with
> > > strings is allowed to have a NULL str (which would give a different code
> > > path than currently, if I were to check for NULL from get_item_string
> > > instead). LISTBOX_lstrcmpiW ends up using CompareStringW which does check
> > > for NULL, so I'm not entirely certain about it.
> > 
> > _InsertString inserts an empty string in this case, but I don't find
> > any tests for this.  Could you add one?
> > 
> > There are also many over places (e.g. the 2nd half of _PaintItem) that
> > could use this helper.  The goal is to have all reads of ->str go through
> > this helper, even in cases where the code is inside a HAS_STRINGS() block.
> > 
> > Huw.
> > 
> 
> Ok, I'll see about the test.

Thanks.

> I didn't want to mess with PaintItem because it
> uses the item pointer directly, so presumably it's valid already.

The item variable should start to disappear from most of these
functions when they start using the helpers.
 
> Since it's used in so many places, I'll use a local variable to hold it at
> the beginning (if valid), and get rid of the "item" variable, since I can
> just check for index instead. Does that sound fine?

Yes, using a local variable to hold the string is absolutely fine.

Huw.