Review Request: Rough implementation of GdipBeginContainer and GdipEndContainer

Vincent Povirk madewokherd+8cd9 at gmail.com
Wed Jul 1 00:27:18 CDT 2009


Comments that become irrelevant when you do this right are helpfully
enclosed in (parentheses).

> First, where should the utility functions and variables go?  Right
> now, they're just thrown in above the functions I'm working on.  It
> seems messy to me to have, for example, low and high (which need to be
> renamed) declared where they are.  Do they belong there (or, rather,
> near the top of graphics.c like some other utility functions), or
> should they be placed elsewhere?

If a symbol is only used within a single .c file and not exported in
the .spec, you should make it a static function. Other than that, you
seem to be declaring functions fine.

>+GraphicsNode* first_graphics_node = NULL;

This should be stored in the GpGraphics struct. Each Graphics object
should have its own independent stack.

(The way you use it is also a little backwards. Adding to the
beginning of a linked list is an O(1) operation, and adding to the end
is O(n). You could use better algorithms if you reverse the order of
the list. But don't worry about that too much. wine/list.h is
doubly-linked so working on either end is O(1).)

>+    dst->worldtrans = GdipAlloc(sizeof(GpMatrix));

Don't do this. Use standard functions to create objects. Just trust
me, it's better that way. Or read the parenthesized comments if you
don't.

>+    high -= 1;
>+    low += 1;
>+    *state = (high << 16) + low;

I have no idea what you're trying to accomplish by doing this. If you
got that by inspecting the values you get from native, I think you're
watching native a little too closely.

>+void delete_graphics(GpGraphics* graphics){
>+    GdipFree(graphics->worldtrans);
>+    GdipFree(graphics->clip);
>+    GdipFree(graphics);
>+}

This is a bad idea. You're defining an additional kind of graphics
object that follows different rules and must be created and destroyed
differently. That's confusing.

Instead of copying the entire graphics struct, you should explicitly
save the fields you need.

(You shouldn't be freeing graphics, since it's a member of a struct
and not a heap-allocated thing whenever delete_object is called.)

(memcpy() of regions is not sound. A region object can refer to any
number of heap-allocated "region elements" that you'll end up sharing
with an existing region. The existing region will then likely free
them if it's modified in the meantime. Your copy will then refer to
freed memory and the program will go to hell.)

(If you copy_graphics TO a real Graphics object, as you do in
pop_graphics_node, you leak the object's current transformation matrix
and clipping region.)

Vincent Povirk



More information about the wine-devel mailing list