[Gdiplus try3 02/16] Implement GdipCreateRegion

[Huw Davies]

Adam Petaccia wrote:
> On Tue, 2008-07-22 at 11:11 +0100, Huw Davies wrote:

>>> +} RegionElement;
>> It would be better to avoid the mixed upper/lower case names, since
>> this makes it look like they're win32 api structures.  Something like
>> typedef struct region_element region_element is nicer.
>> Same goes for the element names like pathHeader etc.
> 
> Will fix. Question: would it be better to declare these structures
> outside, or are inline declarations acceptable? 

inline is fine.


>>> +        DWORD numOps;
>> A better name for numOps would be num_child_nodes.  This is the total
>> number of children under the root node (and is equal to twice the
>> number of ops).  I should update my comment at the beginning of
>> region.c to reflect this.
> 
> Will do. How about just num_children?

Works for me.

> 
>>> +    } header;
>>> +    RegionElement *node;
>> How about actually embedding the structure here rather than a pointer to
>> it?  Since you'll always have a root node there's no need to allocate
>> it explicitly.  And let's call it 'root' not 'node'.
>> Something like region_element root 
> 
> The CombineRegion* code takes advantage over the fact that they are
> pointers and can be shuffled around, rather than
> shuffling/cloning/memcpy'ing potentially large amounts of data around.
> 
> Also, because of the recursive nature of CombineMode, I found it much
> easier to deal with nodes separately (and recursively when necessary)
> than the whole Region at a time.

It should only be a question of how you start your recursion.  Obviously 
the CombineRegion stuff will have a alloc and memcpy the old root node 
to 'left', but then you don't need to alloc the new root node.

>> Are you sure RegionType is in the win32 api?  I can't find this is my
>> gdiplusenums.h.  If it isn't then it should be put in
>> gdiplus_private.h (or even just in region.c if nothing else uses it)
>> and renamed to enum region_type.
> 
> I'll move it to region.c.

Great.

>> Again, I think you'd be better off submitting a couple of patches at a
>> time.
> 
> Sorry, its kind of painful when the work is mostly done, but the problem
> is getting it pushed. I'm slightly behind where I want to be on my GSoC
> schedule, and I was trying to catch up. I'll slow down.

I think you'll actually find it quicker in the long run to have smaller 
patch dumps, that way you'll spend less time re-doing a whole load of 
patches many times.

Huw.