February 2014 - Wine-Devel - list.winehq.org

Re: Patch to fix bug 34324

by Zhenbo Li

2014-02-25 20:33 GMT+08:00 Bruno Jesus <00cpxxx(a)gmail.com>: > > >> BTW, in irc channel, Stefand suggested me to avoid SetLastError(). >> If I have to do that, could I change SHELL_DeleteDirectoryW() >> from BOOL to DWORD? > > If BOOL is no longer enough and the function is internal I see no > problem in changing it, but there are other calls to this function so > you would have to review and update then accordingly. And yes, > avoiding the SetLastError is better as Windows may not set it too and > the application may not be expecting that. Thank you for your suggest. I checked wine's code again. As SHELL_DeleteDirectoryW() is a recursive function, using DWORD instead of BOOL would cause a big change. So I modified the end of delete_files(). if (!bPathExists) - return ERROR_PATH_NOT_FOUND; + return GetLastError() == ERROR_PATH_NOT_FOUND? + 0x7c: /* DE_INVALIDFILES , which is a legacy in Windows*/ + GetLastError(); In this new patch[1], it would fix *all* todo marks about return value in test_delete(), too. I appreciate your feedbacks. (Thank you very much, Bruno Jesus) [1]: http://bugs.winehq.org/attachment.cgi?id=47644&action=diff -- Have a nice day! Zhenbo Li

Feb. 26, 2014

Re: usp10: Use BOOL type where appropriate

by Nikolay Sivov

On 2/26/2014 14:47, Frédéric Delanoy wrote: > On Wed, Feb 26, 2014 at 11:18 AM, Nikolay Sivov <bunglehead(a)gmail.com> wrote: >> On 2/26/2014 14:11, Frédéric Delanoy wrote: >>> On Wed, Feb 26, 2014 at 8:29 AM, Nikolay Sivov <bunglehead(a)gmail.com> >>> wrote: >>>> On 2/26/2014 11:12, Frédéric Delanoy wrote: >>>>> cf. >>>>> >>>>> http://msdn.microsoft.com/en-us/library/windows/desktop/dd374096%28v=vs.85%… >>>>> --- >>>>> dlls/usp10/opentype.c | 32 ++++++++++++++++---------------- >>>>> dlls/usp10/shape.c | 36 ++++++++++++++++++------------------ >>>>> dlls/usp10/usp10.c | 26 +++++++++++++------------- >>>>> 3 files changed, 47 insertions(+), 47 deletions(-) >>>>> >>>>> diff --git a/dlls/usp10/opentype.c b/dlls/usp10/opentype.c >>>>> index 166d823..5b3bdc7 100644 >>>>> --- a/dlls/usp10/opentype.c >>>>> +++ b/dlls/usp10/opentype.c >>>>> @@ -645,34 +645,34 @@ void OpenType_GDEF_UpdateGlyphProps(ScriptCache >>>>> *psc, const WORD *pwGlyphs, cons >>>>> { >>>>> case 0: >>>>> case BaseGlyph: >>>>> - pGlyphProp[i].sva.fClusterStart = 1; >>>>> - pGlyphProp[i].sva.fDiacritic = 0; >>>>> - pGlyphProp[i].sva.fZeroWidth = 0; >>>>> + pGlyphProp[i].sva.fClusterStart = TRUE; >>>>> + pGlyphProp[i].sva.fDiacritic = FALSE; >>>>> + pGlyphProp[i].sva.fZeroWidth = FALSE; >>>>> break; >>>> I think it's a wrong change.Even if msdn says you can use TRUE/FALSE >>>> here, >>> MSDN says explicitly the possible values *are* TRUE and FALSE >>> >>> (http://msdn.microsoft.com/en-us/library/windows/desktop/dd374046%28v=vs.85%…) >> It says a lot of things. > Can you please elaborate? > > E.g. > > fDiacritic > Value indicating if a glyph combines with base characters. > Possible values are defined in the following table. > > Value Meaning > TRUE The glyph does combine with base characters. > FALSE The glyph does not combine with base characters. > > I don't see how much clearer that could be... My point is that you shouldn't use it because structure field type is not BOOL.

Feb. 26, 2014

Re: usp10: Use BOOL type where appropriate

by Frédéric Delanoy

On Wed, Feb 26, 2014 at 11:18 AM, Nikolay Sivov <bunglehead(a)gmail.com> wrote: > On 2/26/2014 14:11, Frédéric Delanoy wrote: >> >> On Wed, Feb 26, 2014 at 8:29 AM, Nikolay Sivov <bunglehead(a)gmail.com> >> wrote: >>> >>> On 2/26/2014 11:12, Frédéric Delanoy wrote: >>>> >>>> cf. >>>> >>>> http://msdn.microsoft.com/en-us/library/windows/desktop/dd374096%28v=vs.85%… >>>> --- >>>> dlls/usp10/opentype.c | 32 ++++++++++++++++---------------- >>>> dlls/usp10/shape.c | 36 ++++++++++++++++++------------------ >>>> dlls/usp10/usp10.c | 26 +++++++++++++------------- >>>> 3 files changed, 47 insertions(+), 47 deletions(-) >>>> >>>> diff --git a/dlls/usp10/opentype.c b/dlls/usp10/opentype.c >>>> index 166d823..5b3bdc7 100644 >>>> --- a/dlls/usp10/opentype.c >>>> +++ b/dlls/usp10/opentype.c >>>> @@ -645,34 +645,34 @@ void OpenType_GDEF_UpdateGlyphProps(ScriptCache >>>> *psc, const WORD *pwGlyphs, cons >>>> { >>>> case 0: >>>> case BaseGlyph: >>>> - pGlyphProp[i].sva.fClusterStart = 1; >>>> - pGlyphProp[i].sva.fDiacritic = 0; >>>> - pGlyphProp[i].sva.fZeroWidth = 0; >>>> + pGlyphProp[i].sva.fClusterStart = TRUE; >>>> + pGlyphProp[i].sva.fDiacritic = FALSE; >>>> + pGlyphProp[i].sva.fZeroWidth = FALSE; >>>> break; >>> >>> I think it's a wrong change.Even if msdn says you can use TRUE/FALSE >>> here, >> >> MSDN says explicitly the possible values *are* TRUE and FALSE >> >> (http://msdn.microsoft.com/en-us/library/windows/desktop/dd374046%28v=vs.85%…) > > It says a lot of things. Can you please elaborate? E.g. fDiacritic Value indicating if a glyph combines with base characters. Possible values are defined in the following table. Value Meaning TRUE The glyph does combine with base characters. FALSE The glyph does not combine with base characters. I don't see how much clearer that could be... Frédéric

Feb. 26, 2014

Re: usp10: Use BOOL type where appropriate

by Michael Stefaniuc

On 02/26/2014 11:11 AM, Frédéric Delanoy wrote: > On Wed, Feb 26, 2014 at 8:29 AM, Nikolay Sivov <bunglehead(a)gmail.com> wrote: >> On 2/26/2014 11:12, Frédéric Delanoy wrote: >>> >>> cf. >>> http://msdn.microsoft.com/en-us/library/windows/desktop/dd374096%28v=vs.85%… >>> --- >>> dlls/usp10/opentype.c | 32 ++++++++++++++++---------------- >>> dlls/usp10/shape.c | 36 ++++++++++++++++++------------------ >>> dlls/usp10/usp10.c | 26 +++++++++++++------------- >>> 3 files changed, 47 insertions(+), 47 deletions(-) >>> >>> diff --git a/dlls/usp10/opentype.c b/dlls/usp10/opentype.c >>> index 166d823..5b3bdc7 100644 >>> --- a/dlls/usp10/opentype.c >>> +++ b/dlls/usp10/opentype.c >>> @@ -645,34 +645,34 @@ void OpenType_GDEF_UpdateGlyphProps(ScriptCache >>> *psc, const WORD *pwGlyphs, cons >>> { >>> case 0: >>> case BaseGlyph: >>> - pGlyphProp[i].sva.fClusterStart = 1; >>> - pGlyphProp[i].sva.fDiacritic = 0; >>> - pGlyphProp[i].sva.fZeroWidth = 0; >>> + pGlyphProp[i].sva.fClusterStart = TRUE; >>> + pGlyphProp[i].sva.fDiacritic = FALSE; >>> + pGlyphProp[i].sva.fZeroWidth = FALSE; >>> break; >> >> I think it's a wrong change.Even if msdn says you can use TRUE/FALSE here, > > MSDN says explicitly the possible values *are* TRUE and FALSE > (http://msdn.microsoft.com/en-us/library/windows/desktop/dd374046%28v=vs.85%…) > >> field is not defined as of 'BOOL' type. > > Because it defines the field as bitfield WORD:1 to get an unsigned > type; Microsoft isn't always consistent about using appropriate field > type, especially when using bitfields BOOL is a signed integer makes no sense as a type for an 1-bit bitfield. Yes, MS is still using it in some places. bye michael

Feb. 26, 2014

Re: usp10: Use BOOL type where appropriate

by Nikolay Sivov

On 2/26/2014 14:11, Frédéric Delanoy wrote: > On Wed, Feb 26, 2014 at 8:29 AM, Nikolay Sivov <bunglehead(a)gmail.com> wrote: >> On 2/26/2014 11:12, Frédéric Delanoy wrote: >>> cf. >>> http://msdn.microsoft.com/en-us/library/windows/desktop/dd374096%28v=vs.85%… >>> --- >>> dlls/usp10/opentype.c | 32 ++++++++++++++++---------------- >>> dlls/usp10/shape.c | 36 ++++++++++++++++++------------------ >>> dlls/usp10/usp10.c | 26 +++++++++++++------------- >>> 3 files changed, 47 insertions(+), 47 deletions(-) >>> >>> diff --git a/dlls/usp10/opentype.c b/dlls/usp10/opentype.c >>> index 166d823..5b3bdc7 100644 >>> --- a/dlls/usp10/opentype.c >>> +++ b/dlls/usp10/opentype.c >>> @@ -645,34 +645,34 @@ void OpenType_GDEF_UpdateGlyphProps(ScriptCache >>> *psc, const WORD *pwGlyphs, cons >>> { >>> case 0: >>> case BaseGlyph: >>> - pGlyphProp[i].sva.fClusterStart = 1; >>> - pGlyphProp[i].sva.fDiacritic = 0; >>> - pGlyphProp[i].sva.fZeroWidth = 0; >>> + pGlyphProp[i].sva.fClusterStart = TRUE; >>> + pGlyphProp[i].sva.fDiacritic = FALSE; >>> + pGlyphProp[i].sva.fZeroWidth = FALSE; >>> break; >> I think it's a wrong change.Even if msdn says you can use TRUE/FALSE here, > MSDN says explicitly the possible values *are* TRUE and FALSE > (http://msdn.microsoft.com/en-us/library/windows/desktop/dd374046%28v=vs.85%…) It says a lot of things. > >> field is not defined as of 'BOOL' type. > Because it defines the field as bitfield WORD:1 to get an unsigned > type; Microsoft isn't always consistent about using appropriate field > type, especially when using bitfields > It doesn't mean we shouldn't be consistent too. My opinion on this is that if it's not BOOL type explicitly - don't use BOOL-kind of constants.

Feb. 26, 2014

Re: usp10: Use BOOL type where appropriate

by Frédéric Delanoy

On Wed, Feb 26, 2014 at 8:29 AM, Nikolay Sivov <bunglehead(a)gmail.com> wrote: > On 2/26/2014 11:12, Frédéric Delanoy wrote: >> >> cf. >> http://msdn.microsoft.com/en-us/library/windows/desktop/dd374096%28v=vs.85%… >> --- >> dlls/usp10/opentype.c | 32 ++++++++++++++++---------------- >> dlls/usp10/shape.c | 36 ++++++++++++++++++------------------ >> dlls/usp10/usp10.c | 26 +++++++++++++------------- >> 3 files changed, 47 insertions(+), 47 deletions(-) >> >> diff --git a/dlls/usp10/opentype.c b/dlls/usp10/opentype.c >> index 166d823..5b3bdc7 100644 >> --- a/dlls/usp10/opentype.c >> +++ b/dlls/usp10/opentype.c >> @@ -645,34 +645,34 @@ void OpenType_GDEF_UpdateGlyphProps(ScriptCache >> *psc, const WORD *pwGlyphs, cons >> { >> case 0: >> case BaseGlyph: >> - pGlyphProp[i].sva.fClusterStart = 1; >> - pGlyphProp[i].sva.fDiacritic = 0; >> - pGlyphProp[i].sva.fZeroWidth = 0; >> + pGlyphProp[i].sva.fClusterStart = TRUE; >> + pGlyphProp[i].sva.fDiacritic = FALSE; >> + pGlyphProp[i].sva.fZeroWidth = FALSE; >> break; > > I think it's a wrong change.Even if msdn says you can use TRUE/FALSE here, MSDN says explicitly the possible values *are* TRUE and FALSE (http://msdn.microsoft.com/en-us/library/windows/desktop/dd374046%28v=vs.85%…) > field is not defined as of 'BOOL' type. Because it defines the field as bitfield WORD:1 to get an unsigned type; Microsoft isn't always consistent about using appropriate field type, especially when using bitfields -- Frédéric Delanoy

Feb. 26, 2014

Re: [PATCH] oleaut32: Implement DISPID_PICT_RENDER dispid

by Nikolay Sivov

On 2/26/2014 13:19, Alistair Leslie-Hughes wrote: > On 26/02/2014 8:05 PM, Nikolay Sivov wrote: >> --- >> dlls/oleaut32/olepicture.c | 51 >> ++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 47 insertions(+), 4 deletions(-) >> > I submitted a similar patch > http://www.winehq.org/pipermail/wine-patches/2011-December/109566.html > > It was rejected because it lacked tests. Without wine-devel comment? Cause I can't find any. > + VariantInit(&varRect); > + hr = VariantChangeTypeEx(&varRect, &pDispParams->rgvarg[0], lcid, 0, VT_UI4 | VT_BYREF); > + if (hr == S_OK) > + pRect = V_INT_PTR(&varRect); This part is wrong.

Feb. 26, 2014

Re: [PATCH] oleaut32: Implement DISPID_PICT_RENDER dispid

by Alistair Leslie-Hughes

On 26/02/2014 8:05 PM, Nikolay Sivov wrote: > --- > dlls/oleaut32/olepicture.c | 51 ++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 47 insertions(+), 4 deletions(-) > I submitted a similar patch http://www.winehq.org/pipermail/wine-patches/2011-December/109566.html It was rejected because it lacked tests. Best Regards Alistair Leslie-Hughes

Feb. 26, 2014

Re: dmime: Do not try to query a NULL direct sound buffer

by Nikolay Sivov

On 2/26/2014 13:03, Aric Stewart wrote: > --- > dlls/dmime/audiopath.c | 33 ++++++++++++++++++--------------- > 1 file changed, 18 insertions(+), 15 deletions(-) Why is it NULL though? The only place I found that sets it is this: --- hr = IDirectSound8_CreateSoundBuffer ((LPDIRECTSOUND8) This->pDirectSound, &desc, &buffer, NULL); if (FAILED(hr)) { HeapFree(GetProcessHeap(), 0, default_path); *ppNewPath = NULL; return DSERR_BUFFERLOST; } default_path->pDSBuffer = buffer; --- That means CreateSoundBuffer() doesn't return error code properly?

Feb. 26, 2014

Re: usp10: Use BOOL type where appropriate

by Nikolay Sivov

On 2/26/2014 11:12, Frédéric Delanoy wrote: > cf. http://msdn.microsoft.com/en-us/library/windows/desktop/dd374096%28v=vs.85%… > --- > dlls/usp10/opentype.c | 32 ++++++++++++++++---------------- > dlls/usp10/shape.c | 36 ++++++++++++++++++------------------ > dlls/usp10/usp10.c | 26 +++++++++++++------------- > 3 files changed, 47 insertions(+), 47 deletions(-) > > diff --git a/dlls/usp10/opentype.c b/dlls/usp10/opentype.c > index 166d823..5b3bdc7 100644 > --- a/dlls/usp10/opentype.c > +++ b/dlls/usp10/opentype.c > @@ -645,34 +645,34 @@ void OpenType_GDEF_UpdateGlyphProps(ScriptCache *psc, const WORD *pwGlyphs, cons > { > case 0: > case BaseGlyph: > - pGlyphProp[i].sva.fClusterStart = 1; > - pGlyphProp[i].sva.fDiacritic = 0; > - pGlyphProp[i].sva.fZeroWidth = 0; > + pGlyphProp[i].sva.fClusterStart = TRUE; > + pGlyphProp[i].sva.fDiacritic = FALSE; > + pGlyphProp[i].sva.fZeroWidth = FALSE; > break; I think it's a wrong change.Even if msdn says you can use TRUE/FALSE here, field is not defined as of 'BOOL' type.

Feb. 26, 2014