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

[Nikolay Sivov]

On 10.03.2017 21:21, Fabian Maurer wrote:
> +    UINT text_size;  /* Length in bytes including null-terminator */
> +    UINT class_size; /* Length in bytes including null-terminator */
> +    struct list entry;
> +}control_info;
> +
> +typedef struct
> +{
> +    DLGTEMPLATE template;
> +    WORD menu;
> +    WORD class;
> +    const WCHAR *title;
> +
> +    /* Not part of actual template */
> +    UINT titleSize; /* Length in bytes including null-terminator */
> +}dialog_header;

About formatting issues, for consistency please add space after bracket,
same goes for 'if' and other places.

"titleSize" -> "title_size".
> +#define MEMCPY_MOVEPTR(target, source, size) memcpy(target, source, size); target += size;

I think this one will be better as a function, also this macro assumes
that target point is char*, which is impossible to guess.
> +static void* align_pointer(void *ptr, unsigned int boundary)
> +{
> +    boundary--;
> +    return (void *)(((UINT_PTR)ptr + boundary) & ~boundary);
> +}

This is usually done with macros in Wine I think, like

((x) + size - 1) & ~(size - 1).
> +LPDLGTEMPLATEW dialog_template_create(dialog_header header, struct list *controls)

Why is 'header' passed by value?
> +    void *template_data_start;

I think it's better to declare this with a type you're going to return.
> +    data_size += sizeof(WORD)*2 + header.titleSize;

Same formatting issue - please add spaces around operator.
> +    /* Align on WORD boundary for the strings */
> +    template_data = align_pointer(template_data, 2);

I don't see how it could be not aligned to WORD, could you explain?

[v3 03/12] comctl32: Added basic implementation for task dialogs and add tests

[Fabian Maurer]

> This is usually done with macros in Wine I think, like
>
> ((x) + size - 1) & ~(size - 1).
> Do we already have a macro like that I should use or should I just define it 
> like that?
> > +LPDLGTEMPLATEW dialog_template_create(dialog_header header, struct list
> > *controls)
> > Why is 'header' passed by value?
> > Because it doesn't really matter. Is there a benefit passing it as pointer?
> > +    /* Align on WORD boundary for the strings */
> > +    template_data = align_pointer(template_data, 2);
>
> I don't see how it could be not aligned to WORD, could you explain?
> I wasn't sure about the alignment of LocalAlloc, so I added that check. Does 
> it guarantee an aligned pointer?

Regarding your points I didn't comment on, I'll fix those in the next version.

Regards
Fabian Maurer

[v3 03/12] comctl32: Added basic implementation for task dialogs and add tests

[Nikolay Sivov]

On 11.03.2017 1:26, Fabian Maurer wrote:
> > This is usually done with macros in Wine I think, like
> >
> > ((x) + size - 1) & ~(size - 1).
> > Do we already have a macro like that I should use or should I just define it 
> > like that?

Not in shared headers, some dlls have their own, for example
ole32/usrmarshal.c does.
> > > +LPDLGTEMPLATEW dialog_template_create(dialog_header header, struct list
> > > *controls)
> > > Why is 'header' passed by value?
> > > Because it doesn't really matter. Is there a benefit passing it as pointer?

Looks odd to me, that's all. I think this could actually be improved by
merging controls list with 'header' fields, and calling it dialog
description or something like that.
> > > +    /* Align on WORD boundary for the strings */
> > > +    template_data = align_pointer(template_data, 2);
> >
> > I don't see how it could be not aligned to WORD, could you explain?
> > I wasn't sure about the alignment of LocalAlloc, so I added that check. Does 
> > it guarantee an aligned pointer?

GlobalAlloc() is documented to align at 8 bytes, and in practice
HeapAlloc() does the same I think.
> Regarding your points I didn't comment on, I'll fix those in the next version.
>
> Regards
> Fabian Maurer

[v3 03/12] comctl32: Added basic implementation for task dialogs and add tests

[Fabian Maurer]

> > > > +    /* Align on WORD boundary for the strings */
> > > > +    template_data = align_pointer(template_data, 2);
> > >
> > > I don't see how it could be not aligned to WORD, could you explain?
> >
> > I wasn't sure about the alignment of LocalAlloc, so I added that check.
> > Does it guarantee an aligned pointer?
>
> GlobalAlloc() is documented to align at 8 bytes, and in practice
> HeapAlloc() does the same I think.
> I see, thanks. Then I'll remove that align_pointer call.

I'll improve the other things you mentioned, too.

Regards
Fabian Maurer