Re: [2/2] gdiplus: Implement GdipBeginContainer2 and GdipEndContainer

[Vincent Povirk]

> +static GpStatus init_container(GraphicsContainerItem** container,
> +        GDIPCONST GpGraphics* graphics){
> +    GpStatus sts;
> +
> +    if(!container || !graphics)
> +        return InvalidParameter;

Since this is an internal function and you know container and graphics
will always be non-NULL when you call it, you don't need to make this
check.
> +    sts = GdipDeleteMatrix(graphics->worldtrans);
> +    if(sts != Ok)
> +        return sts;
> +
> +    sts = GdipCloneMatrix(container->worldtrans, &graphics->worldtrans);
> +    if(sts != Ok)
> +        return sts;

If the delete succeeds but the clone fails, you'll have a Graphics
with a deleted world transform. You should allocate the objects you
need before freeing those attached to the Graphics.

I think that in this case you could get away with copying pointers
rather than creating new objects, and you can assume the delete
functions succeed. Since the container is about to be destroyed, you
would need to make sure the attached matrix and region are not
destroyed with it.
> +    /* did not find a matching container */
> +    if(&container->entry == &graphics->containers)
> +        return Ok;

That's a little strange, even though the test shows it's not an error.
Personally, I would throw in a WARN for this case.

I assume you meant to mark this as [1/2] and the test as [2/2].

This looks good to me. It's almost at the point where I can't predict
whether it will be accepted (as opposed to being sure it won't), but
Alexandre nailed me once for doing a similar "delete before allocating
the replacement" thing. So I assume he cares about that.

-- 
Vincent Povirk