Re: [PATCH 2/2] usp10: Fixed wgBlank, wgDefault, wgInvalid and wgKashida.

[Nikolay Sivov]

On 11/13/2012 18:20, Qian Hong wrote:
> Thanks Aric!
>
> dlls/usp10/usp10.c          |  133 +++++++++++++++++++++++++++++++++++++++++--
>   dlls/usp10/usp10_internal.h |    1 +
>   2 files changed, 128 insertions(+), 6 deletions(-)

I think it's better to avoid macros if possible.
> - if (!sc->sfnt)
> - {
> -
>    if (GetTextMetricsW(hdc, &tmW))
> -
>        sc->sfp.wgBlank = tmW.tmBreakChar;
> -
>    else
> -
>    {
> -
>        ERR("Get wgBlank failed, fallback to Numeric_space.\n");
> -
>        sc->sfp.wgBlank = Numeric_space;
> -
>    }

If it's a normal fallback case it's not an error I guess.
> +{
> +#define Zero_width_space   0x200b /* Zero Width Space */
> +#define Invalid_char3   0xf71b /* Unknow, found by black box testing */
> +#define NON_EXIST_INDEX 0xffff /* Default non exist char index */
> - WCHAR invalid_chars[3] = {Numeric_space, Zero_width_space, Invalid_char3};

I don't see why this can't be hardcoded without macros and one line 
comment about meaning of this.
> +#define Kashida_char    0x0640 /* Kashida */
> - WCHAR kashida_chars[] = {Kashida_char};

Same here, also naming is a bit strange. I think it's enough to name it 
'kashida' and make it 'static const WCHAR'.

[PATCH 2/2] usp10: Fixed wgBlank, wgDefault, wgInvalid and wgKashida.

[Qian Hong]

Hello,

Thanks for comment too!

On Wed, Nov 14, 2012 at 3:25 AM, Nikolay Sivov  wrote:
> I think it's better to avoid macros if possible.
> >

Got it.
> > - if (!sc->sfnt)
> > - {
> > -
> >    if (GetTextMetricsW(hdc, &tmW))
> > -
> >        sc->sfp.wgBlank = tmW.tmBreakChar;
> > -
> >    else
> > -
> >    {
> > -
> >        ERR("Get wgBlank failed, fallback to Numeric_space.\n");
> > -
> >        sc->sfp.wgBlank = Numeric_space;
> > -
> >    }
>
> If it's a normal fallback case it's not an error I guess.

It's not a normal fallback, GetTextMetricsW() should not fail, and I
didn't see fail here for all fonts I tested.
Should I remove the else{} block?
> > +{
> > +#define Zero_width_space   0x200b /* Zero Width Space */
> > +#define Invalid_char3   0xf71b /* Unknow, found by black box testing */
> > +#define NON_EXIST_INDEX 0xffff /* Default non exist char index */
> > - WCHAR invalid_chars[3] = {Numeric_space, Zero_width_space,
> >
> > Invalid_char3};
>
> I don't see why this can't be hardcoded without macros and one line comment
> about meaning of this.

Thanks, will change that.
> > +#define Kashida_char    0x0640 /* Kashida */
> > - WCHAR kashida_chars[] = {Kashida_char};
>
> Same here, also naming is a bit strange. I think it's enough to name it
> 'kashida' and make it 'static const WCHAR'.

Good point, thanks Nikolay.