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.