Re: [Gdiplus try3 02/16] Implement GdipCreateRegion

[Huw Davies]

Hi Adam,

Some comments inlined below:

On Tue, Jul 22, 2008 at 01:22:56AM -0400, Adam Petaccia wrote:
> diff --git a/dlls/gdiplus/gdiplus_private.h b/dlls/gdiplus/gdiplus_private.h
> index e7ca874..9c0246b 100644
> --- a/dlls/gdiplus/gdiplus_private.h
> +++ b/dlls/gdiplus/gdiplus_private.h
> @@ -196,4 +198,40 @@ struct GpFontFamily{
>      WCHAR FamilyName[LF_FACESIZE];
>  };
>
> +typedef struct RegionElement
> +{
> - DWORD type; /* Rectangle, Path, SpecialRectangle, or CombineMode */
> - union
> - {
> -
>    GpRectF rect;
> -
>    struct
> -
>    {
> -
>        GpPath* path;
> -
>        struct
> -
>        {
> -
>            DWORD size;
> -
>            DWORD magic;
> -
>            DWORD pointCount;
> -
>            DWORD storageFlags;
> -
>        } pathHeader;
> -
>    } pathData;
> -
>    struct
> -
>    {
> -
>        struct RegionElement *left;  /* the original region */
> -
>        struct RegionElement *right; /* what *left was combined with */
> -
>    } combine;
> - } elementData;
>
> +} 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.
> +struct GpRegion{
> - struct
> - {
> -
>    DWORD size;
> -
>    DWORD checksum;
> -
>    DWORD magic;
> -
>    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.
> - } 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
> diff --git a/include/gdiplusenums.h b/include/gdiplusenums.h
> index 53401bd..f3110c9 100644
> --- a/include/gdiplusenums.h
> +++ b/include/gdiplusenums.h
> @@ -308,6 +308,15 @@ enum CombineMode
>      CombineModeComplement
>  };
>
> +enum RegionType
> +{
> - RegionDataRect          = 0x10000000,
> - RegionDataPath          = 0x10000001,
> - RegionDataEmptyRect     = 0x10000002,
> - RegionDataInfiniteRect  = 0x10000003,
>
> +};
> -
> -
>
> enum FlushIntention
>  {
>      FlushIntentionFlush = 0,
> @@ -353,6 +362,7 @@ typedef enum HotkeyPrefix HotkeyPrefix;
>  typedef enum PenAlignment GpPenAlignment;
>  typedef enum ImageCodecFlags ImageCodecFlags;
>  typedef enum CombineMode CombineMode;
> +typedef enum RegionType RegionType;
>  typedef enum FlushIntention FlushIntention;
>  typedef enum CoordinateSpace CoordinateSpace;

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.

Again, I think you'd be better off submitting a couple of patches at a
time.

Huw.

[Gdiplus try3 02/16] Implement GdipCreateRegion

[Adam Petaccia]

On Tue, 2008-07-22 at 11:11 +0100, Huw Davies wrote:
> Hi Adam,
>
> Some comments inlined below:

Thanks for the review.
> On Tue, Jul 22, 2008 at 01:22:56AM -0400, Adam Petaccia wrote:
> > diff --git a/dlls/gdiplus/gdiplus_private.h b/dlls/gdiplus/gdiplus_private.h
> > index e7ca874..9c0246b 100644
> > --- a/dlls/gdiplus/gdiplus_private.h
> > +++ b/dlls/gdiplus/gdiplus_private.h
> > @@ -196,4 +198,40 @@ struct GpFontFamily{
> >      WCHAR FamilyName[LF_FACESIZE];
> >  };
> >
> > +typedef struct RegionElement
> > +{
> > - DWORD type; /* Rectangle, Path, SpecialRectangle, or CombineMode */
> > - union
> > - {
> > -
> >    GpRectF rect;
> > -
> >    struct
> > -
> >    {
> > -
> >        GpPath* path;
> > -
> >        struct
> > -
> >        {
> > -
> >            DWORD size;
> > -
> >            DWORD magic;
> > -
> >            DWORD pointCount;
> > -
> >            DWORD storageFlags;
> > -
> >        } pathHeader;
> > -
> >    } pathData;
> > -
> >    struct
> > -
> >    {
> > -
> >        struct RegionElement *left;  /* the original region */
> > -
> >        struct RegionElement *right; /* what *left was combined with */
> > -
> >    } combine;
> > - } elementData;
> >
> > +} 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?
> > +struct GpRegion{
> > - struct
> > - {
> > -
> >    DWORD size;
> > -
> >    DWORD checksum;
> > -
> >    DWORD magic;
> > -
> >    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?
> > - } 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.
> > diff --git a/include/gdiplusenums.h b/include/gdiplusenums.h
> > index 53401bd..f3110c9 100644
> > --- a/include/gdiplusenums.h
> > +++ b/include/gdiplusenums.h
> > @@ -308,6 +308,15 @@ enum CombineMode
> >      CombineModeComplement
> >  };
> >
> > +enum RegionType
> > +{
> > - RegionDataRect          = 0x10000000,
> > - RegionDataPath          = 0x10000001,
> > - RegionDataEmptyRect     = 0x10000002,
> > - RegionDataInfiniteRect  = 0x10000003,
> >
> > +};
> > -
> > -
> >
> > enum FlushIntention
> >  {
> >      FlushIntentionFlush = 0,
> > @@ -353,6 +362,7 @@ typedef enum HotkeyPrefix HotkeyPrefix;
> >  typedef enum PenAlignment GpPenAlignment;
> >  typedef enum ImageCodecFlags ImageCodecFlags;
> >  typedef enum CombineMode CombineMode;
> > +typedef enum RegionType RegionType;
> >  typedef enum FlushIntention FlushIntention;
> >  typedef enum CoordinateSpace CoordinateSpace;
>
> 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.
> 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.
> Huw.

[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.