Re: [PATCH 2/2] usp10: Fixed wgBlank, wgDefault, wgInvalid and wgKashida.
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'.
Hello, Thanks for comment too! On Wed, Nov 14, 2012 at 3:25 AM, Nikolay Sivov <bunglehead(a)gmail.com> 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.
participants (2)
-
Nikolay Sivov -
Qian Hong