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