Re: [PATCH 03/18] comctl32: Added basic implementation for task dialogs and add tests

[Fabian Maurer]

Thanks for the reply,
> If this will stay, please use functions, not macros. STR_SIZE does not
> look useful.

Well, I figured it makes the code a bit easier. Should I should just write it 
out everywhere or make it a function?
> Usually there's no need to allocate 'struct list' itself.

You're right, I'll rewrite that.
> You can't free list node like that.

What do you mean? I allocated the list and it's contents with HeapAlloc, so I 
need to free it that way. Or am I missing something?
> This one is already defined in a header.

Thanks, I'll use the headers instead of adding another define for that.
> Static string for that doesn't look right.

Why is that? I've looked into the other code, and strings are often defined as 
static const WCHAR, that was the recommended method, too.
Of course that needs to be changed later, since we (probably) want to take 
localization into account.

Regards,
Fabian Maurer

[PATCH 03/18] comctl32: Added basic implementation for task dialogs and add tests

[Nikolay Sivov]

On 26.02.2017 1:13, Fabian Maurer wrote:
> Thanks for the reply,
> > If this will stay, please use functions, not macros. STR_SIZE does not
> > look useful.
>
> Well, I figured it makes the code a bit easier. Should I should just write it 
> out everywhere or make it a function?

Yes, functions, or in case of SRT_SIZE I'd just unroll it.
> > You can't free list node like that.
>
> What do you mean? I allocated the list and it's contents with HeapAlloc, so I 
> need to free it that way. Or am I missing something?

You need to unlink it first. Also regarding allocations, in comctl32
most of the time internal Alloc/Free functions are used instead of
HeapAlloc(). At the end it means the same thing, but maybe we should
keep the habit, if only for consistency.
> > Static string for that doesn't look right.
>
> Why is that? I've looked into the other code, and strings are often defined as 
> static const WCHAR, that was the recommended method, too.
> Of course that needs to be changed later, since we (probably) want to take 
> localization into account.

Yes, that's what I mean, it should go to resources.
> Regards,
> Fabian Maurer

[PATCH 03/18] comctl32: Added basic implementation for task dialogs and add tests

[Fabian Maurer]

Thanks for the explanation,
> > > You can't free list node like that.
> >
> > What do you mean? I allocated the list and it's contents with HeapAlloc,
> > so I need to free it that way. Or am I missing something?
>
> You need to unlink it first.

Do I really need to unlink it when I'm freeing everything? Calling list_remove 
before freeing all the entries doesn't really help us, does it? We don't 
access the list afterwards anymore.
> > > Static string for that doesn't look right.
> >
> > Why is that? I've looked into the other code, and strings are often
> > defined as static const WCHAR, that was the recommended method, too.
> > Of course that needs to be changed later, since we (probably) want to take
> > localization into account.
>
> Yes, that's what I mean, it should go to resources.

Yes, I have that in mind for the future, but can I leave it that way for the 
first implementation?

Regards,
Fabian Maurer

[PATCH 03/18] comctl32: Added basic implementation for task dialogs and add tests

[Nikolay Sivov]

On 26.02.2017 21:39, Fabian Maurer wrote:
> Thanks for the explanation,
> > > > You can't free list node like that.
> > >
> > > What do you mean? I allocated the list and it's contents with HeapAlloc,
> > > so I need to free it that way. Or am I missing something?
> >
> > You need to unlink it first.
>
> Do I really need to unlink it when I'm freeing everything? Calling list_remove 
> before freeing all the entries doesn't really help us, does it? We don't 
> access the list afterwards anymore.

Yes, you need to unlink. And yes, you do access it, because you keep
iterating through the list after you freed first entry. Take a look how
it's done, this is used a lot all around.
> > > > Static string for that doesn't look right.
> > >
> > > Why is that? I've looked into the other code, and strings are often
> > > defined as static const WCHAR, that was the recommended method, too.
> > > Of course that needs to be changed later, since we (probably) want to take
> > > localization into account.
> >
> > Yes, that's what I mean, it should go to resources.
>
> Yes, I have that in mind for the future, but can I leave it that way for the 
> first implementation?

It's a matter of a single LoadStringW call, once you added a string as a
resource. Make sure it matches how user32 defines it - as "OK", this way
po files will not require translation updates.
> Regards,
> Fabian Maurer

[PATCH 03/18] comctl32: Added basic implementation for task dialogs and add tests

[Fabian Maurer]

Thanks for explaining, I'll implement it like that.

Regards,
Fabian Maurer