From: Huw Davies <huw@codeweavers.com>
Subject: Re: [PATCH v3 3/4] gdi32: Introduce and use debugstr_face helper.
Message-Id: <20200921092832.GE24050@merlot.physics.ox.ac.uk>
Date: Mon, 21 Sep 2020 10:28:32 +0100
In-Reply-To: <c943877d-65ec-dd6f-002b-9e6866f06efd@codeweavers.com>
References: <20200917173045.1315058-1-rbernon@codeweavers.com> <20200917173045.1315058-3-rbernon@codeweavers.com> <20200921090116.GC24050@merlot.physics.ox.ac.uk> <c943877d-65ec-dd6f-002b-9e6866f06efd@codeweavers.com>

On Mon, Sep 21, 2020 at 11:11:23AM +0200, Rémi Bernon wrote:
> On 2020-09-21 11:01, Huw Davies wrote:
> > On Thu, Sep 17, 2020 at 07:30:44PM +0200, Rémi Bernon wrote:
> > > Signed-off-by: Rémi Bernon <rbernon@codeweavers.com>
> > > ---
> > >   dlls/gdi32/freetype.c | 42 ++++++++++++++++++++----------------------
> > >   1 file changed, 20 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/dlls/gdi32/freetype.c b/dlls/gdi32/freetype.c
> > > index 86585882342..3298b31ab74 100644
> > > --- a/dlls/gdi32/freetype.c
> > > +++ b/dlls/gdi32/freetype.c
> > > @@ -288,6 +288,12 @@ typedef struct tagFace {
> > >       struct enum_data *cached_enum_data;
> > >   } Face;
> > > +static inline const char *debugstr_face( Face *face )
> > > +{
> > > +    if (face->file) return wine_dbg_sprintf( "%s (%ld)", debugstr_w(face->file), face->face_index );
> > > +    else return wine_dbg_sprintf( "%p-%p (%ld)", face->font_data_ptr, (char *)face->font_data_ptr + face->font_data_size, face->face_index );
> > > +}
> > 
> > While this patch is fine in principle, I wonder whether the name of
> > the helper is ideal.  We might want a helper to dump the face's name,
> > ppem, etc. at some point, so unless you're planning to extend this
> > helper to do that, then something like debugstr_face_source() might be
> > more appropriate.
> > 
> > Huw.
> > 
> 
> I wasn't planning on adding anything like that, but there's already a
> "DumpFontList" which is probably supposed to do that, and could perhaps be a
> bit more detailed. In the same way as FreeType has the "FaceID" notion for
> its cache (an abstract type that uniquely identifies a face), it could be
> debugstr_faceid, keeping it short. Would that work for you?

Sure, that works.

Huw.