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?
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
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
- /* 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
-
Fabian Maurer -
Nikolay Sivov