Re: windowscodecs: Adjust a type in PngDecoder_Frame_GetColorContexts.
On Sat, 15 Dec 2012, Alexandre Julliard wrote:
Current snapshots of GCC 4.8.0 issue a warning about some of the
new code in windowscodecs/pngformat.c:
pngformat.c: In function 'PngDecoder_Frame_GetColorContexts':
pngformat.c:882:5: warning: passing argument 5 of 'ppng_get_iCCP' from
incompatible pointer type [enabled by default]
if (ppng_get_iCCP(This-png_ptr, This-info_ptr, name,
compression_type,profile, len))
pngformat.c:882:5: note: expected 'png_bytepp' but argument is of type
'char **'
The patch below addresses this.
That will break on older libpng.
Do you have an idea on how to work around the warning (on newer libpng)?
The patch below does it for me, but there may be a better one...
Gerald
ChangeLog:
Add cast to avoid compiler warning (GCC 4.8) in
PngDecoder_Frame_GetColorContexts.
diff --git a/dlls/windowscodecs/pngformat.c b/dlls/windowscodecs/pngformat.c
index 4183d8e..0d238d5 100644
--- a/dlls/windowscodecs/pngformat.c
+++ b/dlls/windowscodecs/pngformat.c
@@ -879,7 +879,7 @@ static HRESULT WINAPI
PngDecoder_Frame_GetColorContexts(IWICBitmapFrameDecode *i
EnterCriticalSection(This-lock);
-if (ppng_get_iCCP(This-png_ptr, This-info_ptr, name, compression_type,
profile, len))
+if (ppng_get_iCCP(This-png_ptr, This-info_ptr, name, compression_type,
(BYTE**)profile, len))
{
if (cCount ppIColorContexts)
{
Re: mshtml: Set outgoing pass-by-address variable in an error case case within confirm_safety_load.
On Thu, 8 Mar 2012, Gerald Pfeifer wrote: I noticed we return in this case, without initializing this variable. Visual inspection indicates we do not seem to access the variable in this error case, but a) better safe than sorry, and b) GCC 4.8 currently warns about it. I generally don't think this patch makes things any better (or worse). If we return an error, the caller should not expect this value to be sane. What's the GCC 4.8 warning? secmgr.c:230:1: warning: 'policy' may be used uninitialized in this function [-Wmaybe-uninitialized] An alternative to silence this would be initializing policy to some default value. Would that be preferrable? I did not see a response to this, but it seems Francois ran into the same in the meantime and addressed it similarly, so this can be closed: http://www.winehq.org/pipermail/wine-cvs/2012-July/088865.html (http://www.winehq.org/pipermail/wine-patches/2012-March/112089.html was my original submission.) Gerald
Re: mshtml: Set outgoing pass-by-address variable in an error case case within confirm_safety_load.
On Thu, 8 Mar 2012, Jacek Caban wrote: I noticed we return in this case, without initializing this variable. Visual inspection indicates we do not seem to access the variable in this error case, but a) better safe than sorry, and b) GCC 4.8 currently warns about it. I generally don't think this patch makes things any better (or worse). If we return an error, the caller should not expect this value to be sane. What's the GCC 4.8 warning? secmgr.c:230:1: warning: 'policy' may be used uninitialized in this function [-Wmaybe-uninitialized] An alternative to silence this would be initializing policy to some default value. Would that be preferrable? Gerald
Re: advpack: Only do_ocx_reg (and thus DllRegisterServer) from RegisterOCX when 'N' is passed as a flag. Clarify documentation.
Hi James, On Thu, 13 May 2010, Gerald Pfeifer wrote: Would you feel more comfortable leaving the documentation as is and me just suggesting the following? if(strchrW(str_flags,'I')) hr = do_ocx_reg(hm, TRUE); to replace hr = do_ocx_reg(hm, TRUE); ? Or would you prefer to submit a patch to clarify the documentation (copying me) and based on that I hack the code? That way we'd ensure that the former is sufficiently clear and I assume you'll verify whether the code matches that? I just realized I did not see a response to this. How about the patch below that actually matches current documentation (which the current code does not seem to)? Gerald ChangeLog: Only register if I has been passed as a flag. --- dlls/advpack/advpack.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/dlls/advpack/advpack.c b/dlls/advpack/advpack.c index 112d38a..c57933b 100644 --- a/dlls/advpack/advpack.c +++ b/dlls/advpack/advpack.c @@ -519,7 +519,8 @@ HRESULT WINAPI RegisterOCX(HWND hWnd, HINSTANCE hInst, LPCSTR cmdline, INT show) if (!hm) goto done; -hr = do_ocx_reg(hm, TRUE); +if(strchrW(str_flags,'I')) +hr = do_ocx_reg(hm, TRUE); done: FreeLibrary(hm); -- 1.7.6
Re: wpp: Shed an unused parameter from generic_msg. (RESEND)
On Thu, 8 Sep 2011, Alexandre Julliard wrote:
It's still wrong, the parameter is used inside the #ifdef.
Fair enough! How about the patch below then? Tested with and
without WANT_NEAR_INDICATION defined this time.
Or would you rather see a patch that remove WANT_NEAR_INDICATION
altogether?
Gerald
diff --git a/libs/wpp/preproc.c b/libs/wpp/preproc.c
index 99934d6..996c0f8 100644
--- a/libs/wpp/preproc.c
+++ b/libs/wpp/preproc.c
@@ -684,7 +684,11 @@ int pp_get_if_depth(void)
/* #define WANT_NEAR_INDICATION */
-static void generic_msg(const char *s, const char *t, const char *n, va_list
ap)
+static void generic_msg(const char *s, const char *t,
+#ifdef WANT_NEAR_INDICATION
+const char *n,
+#endif
+va_list ap)
{
fprintf(stderr, %s:%d:%d: %s: , pp_status.input ? pp_status.input :
stdin,
pp_status.line_number, pp_status.char_number, t);
@@ -709,13 +713,21 @@ end:
static void wpp_default_error(const char *file, int line, int col, const char
*near, const char *msg, va_list ap)
{
+#ifdef WANT_NEAR_INDICATION
generic_msg(msg, Error, near, ap);
+#else
+ generic_msg(msg, Error, ap);
+#endif
exit(1);
}
static void wpp_default_warning(const char *file, int line, int col, const
char *near, const char *msg, va_list ap)
{
+#ifdef WANT_NEAR_INDICATION
generic_msg(msg, Warning, near, ap);
+#else
+ generic_msg(msg, Warning, ap);
+#endif
}
static const struct wpp_callbacks default_callbacks =
Re: d3dx9_36: Push down the scope of two variables. (RESEND)
On Tue, 5 Jul 2011, Alexandre Julliard wrote: This does not fix a bug or add a feature, but it makes the coder easier to follow. Narrower scope tends to be a good thing for variables, both for human readers and compilers (though modern ones should not need the hint). Not necessarily, that's a matter of taste (IOW, not something you should fix in other people's code). The reason I came up with this patch is that trying to debug something I got lost a bit, and reducing the scope of these variables really helped make things easier to follow. When you are referring to other people's code, according to git log Tony has been the primary contributor, so that would be his call then? Tony, this is http://www.winehq.org/pipermail/wine-patches/2011-July/103866.html -- what's your take? Gerald
Re: urlmon: Fix incorrect pointer arithmetic (too conversative) in map_url_to_zone. (RESEND^2)
A friendly soul pointed out to me that I had missed three(!) responses
to my patch which went to wine-devel@. Sorry about that, I'll see to
set up some mail filters to prevent that from happening.
For the record, somehow I got mislead re the semantics of sizeof, and
that after some 25 years of programming C. :-(
Sorry!
Gerald
On Wed, 15 Jun 2011, Gerald Pfeifer wrote:
Anything wrong with this patch? The original code surely looks wrong?
The difference between two pointers (of the same type) is the number
of elements, not the number of bytes. Thus the code below was way
incorrect, luckily only too conversative.
Gerald
---
dlls/urlmon/sec_mgr.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/dlls/urlmon/sec_mgr.c b/dlls/urlmon/sec_mgr.c
index 7b4bb35..75850ee 100644
--- a/dlls/urlmon/sec_mgr.c
+++ b/dlls/urlmon/sec_mgr.c
@@ -529,7 +529,7 @@ static HRESULT map_url_to_zone(LPCWSTR url, DWORD *zone,
LPWSTR *ret_url)
hres = CoInternetParseUrl(secur_url, PARSE_PATH_FROM_URL, 0, path,
sizeof(path)/sizeof(WCHAR), size, 0);
-if(SUCCEEDED(hres) (ptr = strchrW(path, '\\')) ptr-path
sizeof(root)/sizeof(WCHAR)) {
+if(SUCCEEDED(hres) (ptr = strchrW(path, '\\')) ptr-path
sizeof(root)) {
UINT type;
memcpy(root, path, (ptr-path)*sizeof(WCHAR));
Re: wineoss.drv: Support platforms that do not feature AFMT_FLOAT.
On Mon, 9 May 2011, Andrew Eikum wrote: Anyways this is a tiny hassle, so I'm fine with it. Perhaps you should consider filing a FreeBSD bug? Sure thing, Andrew! That's sounds like a good idea (no pun intended :-). http://www.freebsd.org/cgi/query-pr.cgi?pr=157050 Gerald
Re: wineoss.drv: Add mmdevapi driver.
On Mon, 9 May 2011, Andrew Eikum wrote: Makes sense to me. Thanks. Do this patch and the float format patch together fix your build issues, Gerald? The short answer is: Yes! The long answer is that here have been three different issues around OSS on current versions of FreeBSD where out of the box Wine would not build. 1. Lack of AFMT_FLOAT This has been addressed by Wine commit bed73e9e736ef2376e762730a30ad5f2611f969c of mine which only uses this if present. I also filed the following report against FreeBSD: http://www.freebsd.org/cgi/query-pr.cgi?pr=157050 2. Lack of AFMT_S24_PACKED This has been addressed by Wine commit 051b64b66f82801d43016068639468575dc3974e of mine which replaced it with AFMT_S24_LE. I have not filled a bug against FreeBSD since according to http://manuals.opensound.com/developer/AFMT_S24_PACKED.html this does not appear all too useful to begin with? 3. Lack of SNDCTL_DSP_HALT. This is addressed by my latest patch that uses the equivalent, but older and deprecated, SNDCTL_DSP_RESET in case. I also submitted a patch against FreeBSD along the same lines: http://www.freebsd.org/cgi/query-pr.cgi?pr=156874 As you can see, all three issues have been addressed in Wine now and for two of them I filed bugs against FreeBSD (one with a patch). :-) Thanks for your guidance and support around this! Gerald
Re: TestBot job 11094 results: Re: comctl32/tests: Add casts to avoid comparison of different int types. [take 2]
On Mon, 23 May 2011, Marvin wrote: VM StatusNumber of test failures WINEBUILDcompleted WNT4WSSP6completed 0 W2KPROSP4completed 0 WXPPROSP3completed 0 W2K3R2SESP2 failed WVISTAADMcompleted 0 W2K8SE completed 0 W7PROcompleted 0 W7PROX64 completed 0 W7PROX64 completed 0 For the record, I plead not guilty as far as the following goes. === W2K3R2SESP2 (32 bit tab) === Can't copy exe to VM: Failed to resolve host :-) Gerald
Re: comctl32/tests: Add casts to avoid comparison of different int types. [take 2]
On Wed, 16 Feb 2011, Alexandre Julliard wrote:
This is the first hunk of a patch a few days ago; testbot.winehq.org
thinks it's fine: https://testbot.winehq.org/JobDetails.pl?Key=9291
You don't need casts. If the variable has the wrong type you can change
that.
At first I didn't realize what you had in mind, but now I believe the
patch below does this. :-)
Gerald
ChangeLog:
comctl32/tests: Rewrite a test in test_TCS_OWNERDRAWFIXED to avoid a type
mismatch or cast.
---
dlls/comctl32/tests/tab.c |7 ---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/dlls/comctl32/tests/tab.c b/dlls/comctl32/tests/tab.c
index 8b64764..7f5bbb7 100644
--- a/dlls/comctl32/tests/tab.c
+++ b/dlls/comctl32/tests/tab.c
@@ -1313,6 +1313,7 @@ static void test_TCM_SETITEMEXTRA(void)
static void test_TCS_OWNERDRAWFIXED(void)
{
LPARAM lparam, lparam2;
+ULONG_PTR itemdata;
TCITEMA item;
HWND hTab;
BOOL ret;
@@ -1335,9 +1336,9 @@ static void test_TCS_OWNERDRAWFIXED(void)
ShowWindow(hTab, SW_SHOW);
RedrawWindow(hTab, NULL, 0, RDW_UPDATENOW);
-lparam = 0;
-memset(lparam, 0xde, 4);
-ok(g_drawitem.itemData == lparam, got %lx, expected %lx\n,
g_drawitem.itemData, lparam);
+itemdata = 0;
+memset(itemdata, 0xde, 4);
+ok(g_drawitem.itemData == itemdata, got %lx, expected %lx\n,
g_drawitem.itemData, itemdata);
DestroyWindow(hTab);
--
1.7.4.1
Re: gdi32: Remove a set but unused variable
I admit I am surprised that Commit: 46988651d91fbea7f611287e5d8320239e72108d URL: http://source.winehq.org/git/wine.git/?a=commit;h=46988651d91fbea7f611287e5d8320239e72108d Author: Nicolas Le Cam niko.lecam at gmail.com Date: Fri Apr 29 23:57:09 2011 +0200 gdi32: Remove a set but unused variable. has been applied, for I had proposed this very patch a year ago when Alexandre had rejected it with It seems pretty clear that it should be used, not removed. [ http://www.winehq.org/pipermail/wine-devel/2010-May/083517.html ] I tried to take that into account by updating the patch accordingly http://www.winehq.org/pipermail/wine-patches/2010-May/088317.html which, unexplicably to me, broke the tests so it was not applied: http://www.winehq.org/pipermail/wine-devel/2010-May/083850.html Now my original patch was re-submitted by Nicolas and applied. Was Alexandre's analysis incorrect in the original case, did he just fail to catch the mistake this time around, or is it something else? Gerald
Re: wineoss.drv: Add mmdevapi driver.
On Fri, 6 May 2011, Andrew Eikum wrote:
If Wine is trying to build wineoss.drv, then you must have oss_sysinfo.
If you have oss_sysinfo, then you have OSSv4. If you have OSSv4, then
you should have all of the above symbols, but apparently you don't. So I
suspect it's something amiss with your OSSv4 implementation, and that's
where I would begin investigating.
Does that help you diagnose the problem?
Yes, thanks a lot! Based on yours and Farncois' input, I did some
research and found the following, among others
http://manuals.opensound.com/developer/AFMT_S24_PACKED.html
and consulting Google Code Search I find that also other applications
do something like the following here (mplayer, for example).
Suggested patch below.
Gerald
ChangeLog:
Use AFMT_S24_LE instead of AFMT_S24_PACKED.
diff --git a/dlls/wineoss.drv/mmdevdrv.c b/dlls/wineoss.drv/mmdevdrv.c
index ff21cd6..789cb1e 100644
--- a/dlls/wineoss.drv/mmdevdrv.c
+++ b/dlls/wineoss.drv/mmdevdrv.c
@@ -500,7 +500,7 @@ static int get_oss_format(const WAVEFORMATEX *fmt)
case 16:
return AFMT_S16_LE;
case 24:
-return AFMT_S24_PACKED;
+return AFMT_S24_LE;
case 32:
return AFMT_S32_LE;
}
@@ -953,7 +953,7 @@ static HRESULT WINAPI AudioClient_GetMixFormat(IAudioClient
*iface,
}else if(formats AFMT_S32_LE){
fmt-Format.wBitsPerSample = 32;
fmt-SubFormat = KSDATAFORMAT_SUBTYPE_PCM;
-}else if(formats AFMT_S24_PACKED){
+}else if(formats AFMT_S24_LE){
fmt-Format.wBitsPerSample = 24;
fmt-SubFormat = KSDATAFORMAT_SUBTYPE_PCM;
}else{
Re: wineoss.drv: Add mmdevapi driver.
On Fri, 29 Apr 2011, Gerald Pfeifer wrote: commit be332326ba8fc3def406c5f29adf04fbe9a83976 Author: Andrew Eikum aei...@codeweavers.com Date: Wed Apr 27 09:12:36 2011 -0500 is causing the following build failures on my nightly FreeBSD testers: mmdevdrv.c:463:20: error: 'AFMT_S24_PACKED' undeclared (first use in this function) mmdevdrv.c:476:16: error: 'AFMT_FLOAT' undeclared (first use in this function) mmdevdrv.c:854:24: error: 'AFMT_FLOAT' undeclared (first use in this function) mmdevdrv.c:863:24: error: 'AFMT_S24_PACKED' undeclared (first use in this function) mmdevdrv.c:1084:24: error: 'SNDCTL_DSP_HALT' undeclared (first use in this function) Looks like some autoconfigury is not working or incomplete? This has now been broken for a week; when are you planning on fixing this, Andrew? If not, any pointers on how to best approach this? I checked, and AFMT_FLOAT is not defined in /usr/include on either my FreeBSD 8.2 tester nor my openSUSE 11.4 machine, whereas other AFMT_ strings are. Similarly for AFMT_S24_PACKED and SNDCTL_DSP_HALT. Gerald
Re: wineoss.drv: Add mmdevapi driver.
I believe commit be332326ba8fc3def406c5f29adf04fbe9a83976 Author: Andrew Eikum aei...@codeweavers.com Date: Wed Apr 27 09:12:36 2011 -0500 is causing the following build failures on my nightly FreeBSD testers: mmdevdrv.c:463:20: error: 'AFMT_S24_PACKED' undeclared (first use in this function) mmdevdrv.c:476:16: error: 'AFMT_FLOAT' undeclared (first use in this function) mmdevdrv.c:854:24: error: 'AFMT_FLOAT' undeclared (first use in this function) mmdevdrv.c:863:24: error: 'AFMT_S24_PACKED' undeclared (first use in this function) mmdevdrv.c:1084:24: error: 'SNDCTL_DSP_HALT' undeclared (first use in this function) Looks like some autoconfigury is not working or incomplete? Gerald
Re: Wine: Use the compiler option -Wpointer-arith if available.
On Sun, 17 Apr 2011, Michael Stefaniuc wrote: Of course it doesn't triggers in Wine as -Wpointer-arith is already enabled. I made heavy use of that when I did my pointer cast killing spree a few years ago. Of course! I had missed the fact that this was not in the list of regular options, but needed more extensive configury due to the string.h situation... Gerald
Possible issue in dlls/d3dx9_36/surface.c?
Matteo et al,
there is some code in dlls/d3dx9_36/surface.c which GCC struggles with
(in the sense of warning about it), and so do I, so I am wondering whether
you can have a look?
Specifically, in point_filter_simple_data we have:
DWORD val = 0, pixel;
/* extract source color components */
if(srcformat-type == FORMAT_ARGB) {
pixel = dword_from_bytes(srcptr, srcformat-bytes_per_pixel);
get_relevant_argb_components(conv_info, pixel, channels);
}
So, pixel is uninitialized, except when we have FORMAT_ARGB.
However, directly below we then have
/* recombine the components */
if(destformat-type == FORMAT_ARGB) val = make_argb_color(conv_info,
channels);
if(colorkey) {
get_relevant_argb_components(ck_conv_info, pixel, channels);
where pixel is passed to get_relevant_argb_components. This is guarded
by colorkey, a parameter to this function, so indeed I can see pixel being
unused here.
(I'd naively propose a patch like the one below; this may not be
sufficient though.)
Gerald
diff --git a/dlls/d3dx9_36/surface.c b/dlls/d3dx9_36/surface.c
index a3e6e68..cc36818 100644
--- a/dlls/d3dx9_36/surface.c
+++ b/dlls/d3dx9_36/surface.c
@@ -701,7 +701,7 @@ static void copy_simple_data(CONST BYTE *src, UINT
srcpitch, POINT srcsize,
DWORD val = 0;
for(x = 0;x minwidth;x++) {
-DWORD pixel;
+DWORD pixel = 0x;
/* extract source color components */
if(srcformat-type == FORMAT_ARGB) {
@@ -768,7 +768,7 @@ static void point_filter_simple_data(CONST BYTE *src, UINT
srcpitch, POINT srcsi
for(x = 0;x destsize.x;x++) {
const BYTE *srcptr = bufptr + (x * srcsize.x / destsize.x) *
srcformat-bytes_per_pixel;
-DWORD val = 0, pixel;
+DWORD val = 0, pixel = 0x;
/* extract source color components */
if(srcformat-type == FORMAT_ARGB) {
Re: winmm: Simplify MCI_DumpCommandTable a bit. (RETRY)
On Wed, 16 Feb 2011, Alexandre Julliard wrote:
The variable is here for documentation, you don't want to remove it. You
can comment it out if it's not used.
Fair enough! Updated patch below.
Gerald
ChangeLog:
winmm: Comment out unused variable in Simplify MCI_DumpCommandTable.
diff --git a/dlls/winmm/mci.c b/dlls/winmm/mci.c
index 30db363..8a889d2 100644
--- a/dlls/winmm/mci.c
+++ b/dlls/winmm/mci.c
@@ -625,7 +625,6 @@ static BOOLMCI_DumpCommandTable(UINT uTbl)
{
const BYTE*lmem;
LPCWSTRstr;
-DWORD flg;
WORD eid;
if (!MCI_IsCommandTableValid(uTbl)) {
@@ -636,9 +635,10 @@ static BOOLMCI_DumpCommandTable(UINT uTbl)
lmem = S_MciCmdTable[uTbl].lpTable;
do {
do {
+ /* DWORD flg; */
str = (LPCWSTR)lmem;
lmem += (strlenW(str) + 1) * sizeof(WCHAR);
- flg = *(const DWORD*)lmem;
+ /* flg = *(const DWORD*)lmem; */
eid = *(const WORD*)(lmem + sizeof(DWORD));
/* TRACE(cmd=%s %08lx %04x\n, debugstr_w(str), flg, eid); */
lmem += sizeof(DWORD) + sizeof(WORD);
Re: comctl32/tests: Add casts to avoid two comparisons of different int types.
On Sun, 13 Feb 2011, Janne Hakonen wrote: Actually, now with your change you are comparing ULONG_PTR to ULONG. With 64 bit binary the left side of comparison is a 64 bit pointer and right side is 32 bit value. Thanks, Janne. I have adjusted, tested and resubmitted the first hunk of the patch now. And this is now the second hunk, also approved by Marvin: https://testbot.winehq.org/JobDetails.pl?Key=9292 Gerald ChangeLog: comctl32/tests: Add another cast to avoid comparison of different int types. diff --git a/dlls/comctl32/tests/tab.c b/dlls/comctl32/tests/tab.c index 4c464e6..14c7b59 100644 --- a/dlls/comctl32/tests/tab.c +++ b/dlls/comctl32/tests/tab.c @@ -1327,7 +1327,7 @@ static void test_TCS_OWNERDRAWFIXED(HWND parent_wnd) ShowWindow(hTab, SW_SHOW); RedrawWindow(hTab, NULL, 0, RDW_UPDATENOW); -ok(*(ULONG_PTR*)g_drawitem.itemData == lparam, got %lx, expected %lx\n, g_drawitem.itemData, lparam); +ok(*(ULONG_PTR*)g_drawitem.itemData == (ULONG_PTR)lparam, got %lx, expected %lx\n, g_drawitem.itemData, lparam); DestroyWindow(hTab);
Re: comctl32/tests: Add two casts to avoid comparisons of different int types.
On Sun, 13 Feb 2011, Marvin wrote: While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=9229 Your paranoid android. === W7PROX64 (64 bit tab) === tab.c:1330: Test failed: got 69cc80, expected dededede Hmm, looking at this my patch changes ok(*(ULONG_PTR*)g_drawitem.itemData == lparam, got %lx, expected %lx\n, gdrawitem.itemData, lparam); to ok(*(ULONG_PTR*)g_drawitem.itemData == (ULONG)lparam, got %lx, expected %lx\n, g_drawitem.itemData, lparam); That is, the only difference is the (ULONG) cast. Given the output above, my patch did not trigger this failure since based on the printing of the two values they are different. Gerald
test_TCS_OWNERDRAWFIXED in dlls/comctl32/tests/tab ?
test_TCS_OWNERDRAWFIXED in dlls/comctl32/tests/tab has the following code: LPARAM lparam, lparam2; : lparam = 0; memset(lparam, 0xde, 4); memset(lparam2, 0xde, sizeof(LPARAM)-1); ok(g_drawitem.itemData == lparam || broken(g_drawitem.itemData == lparam2) /* win98 */, got 0x%lx, expected 0x%lx\n, g_drawitem.itemData, lparam); I have two questions on this. First, why does the memset use the constant 4 and not sizeof(LPARAM)? Second, why initialize lparam with 0 when this is immediately followed by a memset covering it's fully size? Third, lparam2 is partially uninitialized and will have a couple of random bits from all I can tell since lparam2 is never initialized and the memset only covers a portion of it. I'm wondering, should lparam = 0 just read lparam2 = 0? Whatever may be the case, the current code does not look right. Gerald
Re: Improve handling of invalid input in dlls/comctl32/datetime.c
For the record, this is how this has been addressed a few months
after this discussion. That's another way of doing it. :-)
Gerald
commit 1af17844301c4924dd8324cbf2f9c3c1ef0394b2
Author: Huw Davies h...@codeweavers.com
Date: Tue May 4 14:05:13 2010 +0100
comctl32: Silence a few compiler warnings.
diff --git a/dlls/comctl32/datetime.c b/dlls/comctl32/datetime.c
index dc2dd12..2418950 100644
--- a/dlls/comctl32/datetime.c
+++ b/dlls/comctl32/datetime.c
@@ -614,7 +614,7 @@ DATETIME_ReturnFieldWidth (const DATETIME_INFO
*infoPtr, HDC hdc, int count, SHO
lctype = LOCALE_SABBREVMONTHNAME1;
max_count = 12;
break;
- case FULLMONTH:
+ default: /* FULLMONTH */
fall = fld_mon;
lctype = LOCALE_SMONTHNAME1;
max_count = 12;
On Fri, 25 Dec 2009, James McKenzie wrote:
Nikolay Sivov wrote:
On 12/26/2009 00:04, James McKenzie wrote:
Nikolay Sivov wrote:
On 12/25/2009 14:18, Gerald Pfeifer wrote:
Otherwise max_count will be undefined in the following loop may do
interesting things it seems. (Does Coverity diagnose similar items?)
Gerald
ChangeLog:
Improve handling of invalid input in DATETIME_ReturnFieldWidth.
diff --git a/dlls/comctl32/datetime.c b/dlls/comctl32/datetime.c
index c240d4f..08c7082 100644
--- a/dlls/comctl32/datetime.c
+++ b/dlls/comctl32/datetime.c
@@ -38,6 +38,7 @@
*-- FORMATCALLBACK
*/
+#includeassert.h
#includemath.h
#includestring.h
#includestdarg.h
@@ -619,6 +620,9 @@ DATETIME_ReturnFieldWidth (const DATETIME_INFO
*infoPtr, HDC hdc, int count, SHO
lctype = LOCALE_SMONTHNAME1;
max_count = 12;
break;
+default:
+assert(0);
+max_count=0;
}
cx = 0;
Hi, Gerald.
This can't even happen. spec if filtered later with:
---
case THREECHARMONTH:
case FULLMONTH:
case THREECHARDAY:
case FULLDAY:
---
So no default case here.
Even if the default case cannot be reached, it is not a 'bad thing' to
have one, even if it is an error message stating Something went wrong,
we should not get here with an exit code...
Why? There's no default case, treat is as 'if () {} else if () {} etc.'.
It's the same thing to have explicit initializers for all local
variables even if I don't use it before
set some value. It's a pollution, especially if used to silence
analyzer warnings.
Nikolay:
Things can and do go 'wrong'. For instance, you build on Linux, I build
on a Mac. Sometimes the ported programs from UNIX can and will do
'strange' things. If you don't have a method to know that there is an
error, then you go about blaming Wine when it is a poor program port.
Also, it is good programming practice to account for the situations you
don't expect to encounter and to initialize variables that you will be
using. This is not a 'just in case' thing, it can help when program
errors occur to see where in the code an expected value did or did not
happen.
I know, it looks like pollution, but when code goes wrong, and it will,
this makes troubleshooting much easier. I know this from running
Quality Assurance testing for many years and coding myself. My
programming instructor deducted grade points for failing to handle error
and other unexpected conditions.
James McKenzie
ws2_32: Restructure and simplify debugstr_wsaioctl a bit.
On Tue, 28 Sep 2010, Juan Lang wrote:
This mimicks what we already do a few lines above for buf_type.
It's also unneeded:
switch(ioctl 0x1800)
There are only 4 possible combinations of ioctl 0x1800. Three
are covered in this switch statement, and the fourth is eliminated due
to the outer switch statement. You'd be adding dead code.
You're right, good catch. That said, this code is really a bit
tricky and looks more complicated than it needs be.
How about the patch below which avoids the nested switch statement,
allows the compiler to realize what's going on, and makes the code
smaller, too. (Not sure why the difference appears that large, but
socket.o goes down from 493528 bytes to 492476 on my tester.)
Gerald
---
dlls/ws2_32/socket.c | 69 ++---
1 files changed, 31 insertions(+), 38 deletions(-)
diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c
index 3d85bad..f7607c5 100644
--- a/dlls/ws2_32/socket.c
+++ b/dlls/ws2_32/socket.c
@@ -2967,14 +2967,24 @@ char* WINAPI WS_inet_ntoa(struct WS_in_addr in)
static const char *debugstr_wsaioctl(DWORD ioctl)
{
+const char *buf_type, *family;
+
switch(ioctl 0x1800)
{
-case WS_IOC_UNIX:
+case WS_IOC_WS2:
+family = IOC_WS2;
+break;
+case WS_IOC_PROTOCOL:
+family = IOC_PROTOCOL;
+break;
+case WS_IOC_VENDOR:
+family = IOC_VENDOR;
+break;
+default: /* WS_IOC_UNIX */
{
BYTE size = (ioctl 16) WS_IOCPARM_MASK;
char x = (ioctl 0xff00) 8;
BYTE y = ioctl 0xff;
-const char *buf_type;
char args[14];
switch (ioctl (WS_IOC_VOID|WS_IOC_INOUT))
@@ -2998,47 +3008,30 @@ static const char *debugstr_wsaioctl(DWORD ioctl)
}
return wine_dbg_sprintf(%s(%s), buf_type, args);
}
-default:
-{
-USHORT code = ioctl 0x;
-const char *family, *buf_type;
+}
-/* This switch looks redundant, but isn't: the case WS_IOC_UNIX
- * is handled differently than all others.
- */
-switch(ioctl 0x1800)
-{
-case WS_IOC_WS2:
-family = IOC_WS2;
+/* We are different from WS_IOC_UNIX. */
+switch (ioctl (WS_IOC_VOID|WS_IOC_INOUT))
+{
+case WS_IOC_VOID:
+buf_type = _WSAIO;
break;
-case WS_IOC_PROTOCOL:
-family = IOC_PROTOCOL;
+case WS_IOC_INOUT:
+buf_type = _WSAIORW;
break;
-case WS_IOC_VENDOR:
-family = IOC_VENDOR;
+case WS_IOC_IN:
+buf_type = _WSAIOW;
+break;
+case WS_IOC_OUT:
+buf_type = _WSAIOR;
+break;
+default:
+buf_type = ?;
break;
-}
-switch (ioctl (WS_IOC_VOID|WS_IOC_INOUT))
-{
-case WS_IOC_VOID:
-buf_type = _WSAIO;
-break;
-case WS_IOC_INOUT:
-buf_type = _WSAIORW;
-break;
-case WS_IOC_IN:
-buf_type = _WSAIOW;
-break;
-case WS_IOC_OUT:
-buf_type = _WSAIOR;
-break;
-default:
-buf_type = ?;
-break;
-}
-return wine_dbg_sprintf(%s(%s, %d), buf_type, family, code);
-}
}
+
+return wine_dbg_sprintf(%s(%s, %d), buf_type, family,
+(USHORT)(ioctl 0x));
}
/**
--
1.7.2.2
Re: configure: Use -Wundef if supported by the compiler.
On Mon, 3 Jan 2011, Alexandre Julliard wrote: I verified this does not cause any warning on FreeBSD 8.1 test builds, and all the tools like bison and flex in somewhat current versions. It's broken here with bison 2.4.1: make[1]: Entering directory `/home/julliard/wine/wine/libs/wpp' gcc -m32 -c -I. -I. -I../../include -I../../include -D_REENTRANT -fPIC -Wall -pipe -fno-strict-aliasing -Wdeclaration-after-statement -Wstrict-prototypes -Wtype-limits -Wundef -Wwrite-strings -Wpointer-arith -Werror -g -O2 -o ppy.tab.o ppy.tab.c ppy.tab.c:336:6: error: YYENABLE_NLS is not defined ppy.tab.c:910:6: error: YYLTYPE_IS_TRIVIAL is not defined make[1]: *** [ppy.tab.o] Error 1 This does not occur with bison 2.4.3. I investigated, and it seems this was actually fixed with bision 2.4.2: http://git.savannah.gnu.org/cgit/bison.git/commit/?id=3d04b5673d26813f3fcec37982ee84d05c3d71aa Checking this via autoconf sounds a bit involved. Do you have a recommendation? Perhaps just wait until Alexandre runs bison 2.4.2 or later on all his machines? ;-) Gerald
Re: Wine: Use GCC's -Wlogical-op if supported.
On Tue, 21 Sep 2010, Alexandre Julliard wrote: I still see a ton of warnings here (gcc 4.4.5), mostly because of the glibc string macros. Hmm, I see. Still, this is really useful to have. How about the patch below as an alternative? Additional platforms can add this when/if they want. This would have to be a compilation check with the appropriate headers, not a platform check. Hmm, that's really a bit tricky given my level of autoconf skills; anyone else who might give this a try? Gerald
Use of uninitialized variable in combine_uri()
Hi Thomas,
the following change of yours
commit bced2e21dbc548ef9d41e3ff11384d7ad964c929
Author: Thomas Mullaly thomas.mull...@gmail.com
Date: Sat Oct 9 11:02:17 2010 -0400
urlmon: Implemented base case for CoInternetCombineIUri.
introduces a new warning, use of uninitialized variable in the line
marked HERE below.
+static HRESULT combine_uri(Uri *base, Uri *relative, DWORD flags, IUri
**result
+Uri *ret;
+HRESULT hr;
+parse_data data;
+
+/* Base case is when the relative Uri has a scheme name,
+ * if it does, then 'result' will contain the same data
+ * as the relative Uri.
+ */
+if(relative-scheme_start -1) {
+DWORD create_flags = 0;
+
+memset(data, 0, sizeof(parse_data));
+
+data.uri = SysAllocString(relative-raw_uri);
+if(!data.uri) {
+IUri_Release(URI(ret)); == HERE
+*result = NULL;
+return E_OUTOFMEMORY;
+}
From all I can tell this is a legitimate warning, that is, the code
really invokes undefined behavior. Would you mind having a look?
Gerald
Re: Wine: Use GCC's -Wlogical-op if supported.
On Mon, 20 Sep 2010, Alexandre Julliard wrote: In the past this has found a dozen or two real issues and lead to some simplifications. Now the tree is clean in that regard, so we can make this a default. I still see a ton of warnings here (gcc 4.4.5), mostly because of the glibc string macros. Hmm, I see. Still, this is really useful to have. How about the patch below as an alternative? Additional platforms can add this when/if they want. Gerald ChangeLog: Use GCC's -Wlogical-op if supported (restricted to FreeBSD for now). diff --git a/configure.ac b/configure.ac index e3220ef..21a06b2 100644 --- a/configure.ac +++ b/configure.ac @@ -1611,6 +1611,9 @@ then WINE_TRY_CFLAGS([-fno-builtin],[AC_SUBST(BUILTINFLAG,-fno-builtin)]) WINE_TRY_CFLAGS([-fno-strict-aliasing]) WINE_TRY_CFLAGS([-Wdeclaration-after-statement]) + if test `uname -s` = FreeBSD; then +WINE_TRY_CFLAGS([-Wlogical-op]) + fi WINE_TRY_CFLAGS([-Wstrict-prototypes]) WINE_TRY_CFLAGS([-Wtype-limits]) WINE_TRY_CFLAGS([-Wwrite-strings]) -- 1.7.2.2
Re: Use GCC's -Wlogical-op if possible
On Mon, 4 Jan 2010, Henri Verbeet wrote: I dont see a reason to take that warning serious, as its not a problem in any case. in kernel32 it just happens because we use a Macro. Taking that case(v==1) out of the Macro leads to harder readable code. So IMHO i would not make -Wlogical-op the default. Not using that macro at all would be even more readable. I'm probably missing something obvious, but it seems to just do a wsprintfW() with #%d as format. I see this has been comitted via b45d4aa161fbbb905fa9142d2757ff3f7952566d. Thanks! Looks like this warning is useful after all. ;-) In fact, doing a build with -Wlogical-op right now it turns out that with the change above and the work last year there aren't any open issues on that front any more and I'll submit a patch to make this used by default. Gerald
krnl386.exe16: Remove stubs for DOSCONF_Device and DOSCONF_Install.DOSCONF_Device and DOSCONF_Install. (RESEND)
On Tue, 25 May 2010, Alexandre Julliard wrote:
An alternate patch would be putting #if 0 around the first occurrence
of loadhigh, too, but this code has been marked dead for more than 11
years, so I really think it can / should go?
It doesn't make sense to parse and not do anything with it. If you want
to remove dead code you can remove essentially the entire file.
I did not feel that brave ;-), but indeed, how about removing those
function skeletons after 11 years as per the patch below?
Gerald
---
dlls/krnl386.exe16/dosconf.c | 45 ++---
1 files changed, 7 insertions(+), 38 deletions(-)
diff --git a/dlls/krnl386.exe16/dosconf.c b/dlls/krnl386.exe16/dosconf.c
index a1500b7..cdbe5a1 100644
--- a/dlls/krnl386.exe16/dosconf.c
+++ b/dlls/krnl386.exe16/dosconf.c
@@ -41,12 +41,10 @@
WINE_DEFAULT_DEBUG_CHANNEL(profile);
-static int DOSCONF_Device(char **confline);
static int DOSCONF_Dos(char **confline);
static int DOSCONF_Fcbs(char **confline);
static int DOSCONF_Break(char **confline);
static int DOSCONF_Files(char **confline);
-static int DOSCONF_Install(char **confline);
static int DOSCONF_Lastdrive(char **confline);
static int DOSCONF_Menu(char **confline);
static int DOSCONF_Include(char **confline);
@@ -58,6 +56,11 @@ static int DOSCONF_Stacks(char **confline);
static int DOSCONF_Buffers(char **confline);
static void DOSCONF_Parse(char *menuname);
+static int DOSCONF_dummy(char **confline)
+{
+return 1;
+}
+
static DOSCONF DOSCONF_config =
{
'E', /* lastdrive */
@@ -92,12 +95,12 @@ static const TAG_ENTRY DOSCONF_tag_entries[] =
{
{ ;, NULL },
{ REM , NULL },
-{ DEVICE, DOSCONF_Device },
+{ DEVICE, DOSCONF_dummy },
{ [, DOSCONF_Menu },
{ SUBMENU, NULL },
{ MENUDEFAULT, DOSCONF_Menu },
{ INCLUDE, DOSCONF_Include },
-{ INSTALL, DOSCONF_Install },
+{ INSTALL, DOSCONF_dummy },
{ DOS, DOSCONF_Dos },
{ FCBS, DOSCONF_Fcbs },
{ BREAK, DOSCONF_Break },
@@ -143,25 +146,6 @@ static int DOSCONF_JumpToEntry(char **pconfline, char
separator)
return 1;
}
-static int DOSCONF_Device(char **confline)
-{
-int loadhigh = 0;
-
-*confline += 6; /* strlen(DEVICE) */
-if (!(strncasecmp(*confline, HIGH, 4)))
-{
- loadhigh = 1;
- *confline += 4;
- /* FIXME: get DEVICEHIGH parameters if avail ? */
-}
-if (!(DOSCONF_JumpToEntry(confline, '='))) return 0;
-TRACE(Loading device '%s'\n, *confline);
-#if 0
-DOSMOD_LoadDevice(*confline, loadhigh);
-#endif
-return 1;
-}
-
static int DOSCONF_Dos(char **confline)
{
*confline += 3; /* strlen(DOS) */
@@ -234,21 +218,6 @@ static int DOSCONF_Files(char **confline)
return 1;
}
-static int DOSCONF_Install(char **confline)
-{
-#if 0
-int loadhigh = 0;
-#endif
-
-*confline += 7; /* strlen(INSTALL) */
-if (!(DOSCONF_JumpToEntry(confline, '='))) return 0;
-TRACE( Installing '%s'\n, *confline );
-#if 0
-DOSMOD_Install(*confline, loadhigh);
-#endif
-return 1;
-}
-
static int DOSCONF_Lastdrive(char **confline)
{
*confline += 9; /* strlen(LASTDRIVE) */
--
1.7.2.2
Re: autoconf 2.62 failure due to AS_VAR_IF
On Tue, 18 May 2010, Gerald Pfeifer wrote:
Using autoconf 2.62 on my primary test system I am getting:
configure:26081: error: possibly undefined macro: AS_VAR_IF
Indeed use of AS_VAR_IF appears relatively new, and it seems this may be
similar to AS_VAR_APPEND where we have the following in configure.ac:
dnl autoconf versions before 2.63b don't have AS_VAR_APPEND
m4_ifdef([AS_VAR_APPEND],,[as_fn_append () { eval $[1]=\$$[1]\$[2]; }
AC_DEFUN([AS_VAR_APPEND],[as_fn_append $1 $2])])dnl
I assume we need something similar here, too? I am really far from
an autoconf hacker -- is this something one of you could look into?
Indeed this was specific to autoconf 2.62 vs 2.64. Running autoconfig
2.67 on said tester now, everything works smoothly.
Gerald
Re: user32: warnings (around uninitialized variables).
On Tue, 20 Jul 2010, Alexandre Julliard wrote:
@@ -388,6 +388,8 @@ static int DIB_GetBitmapInfo( const BITMAPINFOHEADER
*header, LONG *width,
*compr = header-biCompression;
return 1;
}
+
+*width = *height = 0;
ERR((%d): unknown/wrong size for header\n, header-biSize );
return -1;
}
0 is not valid for a bitmap, these should never be used in the error
case.
Well, I'm not sure this works right now: DIB_GetBitmapInfo is called
at the beginning of BITMAP_Load and returns -1 in the case of failure,
which it detects if header-biSize is inappropriate. Somehow this is
never checked, however, it seems?B
So, how about the following patch? Already earlier in BITMAP_Load
we return 0 in the case of problems, so callers should be prepared
for it.
Note my the patch also updated the documentation a bit, and that piece
I just resubmitted separately.
Gerald
ChangeLog:
user32: Fix error handling in BITMAP_Load.
---
dlls/user32/cursoricon.c |6 ++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/dlls/user32/cursoricon.c b/dlls/user32/cursoricon.c
index 9dc6d2b..acae302 100644
--- a/dlls/user32/cursoricon.c
+++ b/dlls/user32/cursoricon.c
@@ -2257,6 +2257,12 @@ static HBITMAP BITMAP_Load( HINSTANCE instance, LPCWSTR
name,
memcpy(scaled_info, fix_info, size);
bm_type = DIB_GetBitmapInfo( fix_info-bmiHeader, width, height,
bpp_dummy, compr_dummy);
+if (bm_type == -1)
+{
+WARN(Invalid bitmap format!\n);
+goto end_close;
+}
+
if(desiredx != 0)
new_width = desiredx;
else
--
1.7.2.2
Re: Wine without X? Failing in dlls/winex11.drv
On Sun, 14 Feb 2010, James McKenzie wrote: Please take time to read through the remainder of the posts here, even though you've been here for a while. It is NOT possible to build Wine without some sort of X on the build computer right now. Yep, I know. Which is why I wondered that configure did not bail out on my (the system in question was after an upgrade, and apparently something had gone wrong there. Which is why I asked here. ;-) On Sun, 14 Feb 2010, Alexandre Julliard wrote: It's definitely supposed to fail in configure. That's handled by the standard autoconf built-in check for X. Please try to figure out why it doesn't catch that case. I spent an hour or so back then, and again an hour today and could not get this to reproduce. Presumably it's some weird combination of things that I had addressed and then failed to go back to. So, the only outcome of this is the little formatting patch I just submitted: http://www.winehq.org/pipermail/wine-patches/2010-September/093427.html Sorry, Gerald
Re: dlls/d3dx9_36/bytecodewriter.c oddity
On Thu, 19 Aug 2010, Matteo Bruni wrote: Hmm, so srcidx is unused. Yes, that piece of code is useless now, it's a remnant of an older version of that function where the source register was handled by some ad-hoc code, which I since then replaced with a call to the generic This-funcs-srcreg(). The function is correct otherwise, there are some tests to confirm that. In my opinion you can send the patch as is. Alternatively you can replace that piece of code with just a check for instr-src[0].regnum != T[0123]_REG, to keep the WARN, but it's not worth the effort probably. Thanks a lot Matteo for your guidance! Based on this, I'll formally submit this to wine-patches. Gerald
dlls/d3dx9_36/bytecodewriter.c oddity
Metteo et al,
I noticed d3dx9_36/bytecodewriter.c can be simplified as follows, but
somewhat tells me this may rather point out a problem somewhere in that
code, so I am not sending this to wine-patches.
(The only functional difference should be the missing WARN in some cases,
and I could restore that if desired, but...see above.)
Thoughts? Guidance?
Gerald
diff --git a/dlls/d3dx9_36/bytecodewriter.c
b/dlls/d3dx9_36/bytecodewriter.c
index 07f96e7..e7bd8a5 100644
--- a/dlls/d3dx9_36/bytecodewriter.c
+++ b/dlls/d3dx9_36/bytecodewriter.c
@@ -1035,7 +1035,7 @@ static const struct bytecode_backend vs_1_x_backend = {
static void instr_ps_1_0123_texld(struct bc_writer *This,
const struct instruction *instr,
struct bytecode_buffer *buffer) {
-DWORD idx, srcidx;
+DWORD idx;
struct shader_reg reg;
DWORD swizzlemask;
@@ -1074,17 +1074,6 @@ static void instr_ps_1_0123_texld(struct bc_writer *This,
/* map the temp dstreg to the ps_1_3 texture temporary register */
This-funcs-dstreg(This, instr-dst, buffer, instr-shift,
instr-dstmod);
} else if(instr-src[0].type == BWRITERSPR_TEMP) {
-if(instr-src[0].regnum == T0_REG) {
-srcidx = 0;
-} else if(instr-src[0].regnum == T1_REG) {
-srcidx = 1;
-} else if(instr-src[0].regnum == T2_REG) {
-srcidx = 2;
-} else if(instr-src[0].regnum == T3_REG) {
-srcidx = 3;
-} else {
-WARN(Invalid address data source register r%u\n,
instr-src[0].regnum);
-}
swizzlemask = (3 BWRITERVS_SWIZZLE_SHIFT) |
(3 (BWRITERVS_SWIZZLE_SHIFT + 2)) |
Re: comctl32: Simplify is_textT by omitting isW.
On Mon, 10 May 2010, Nikolay Sivov wrote: -static inline BOOL is_textT(LPCWSTR text, BOOL isW) +static inline BOOL is_textT(LPCWSTR text) There's no reason to keep this helper then. It's better to rename is_textW() to is_text() and remove is_textT(). Excellent idea. I'll submit a patch to the extent in a minute (and sorry that this has taken three months). Gerald
autoconf 2.62 failure due to AS_VAR_IF
Using autoconf 2.62 on my primary test system I am getting:
configure:26081: error: possibly undefined macro: AS_VAR_IF
Indeed use of AS_VAR_IF appears relatively new, and it seems this may be
similar to AS_VAR_APPEND where we have the following in configure.ac:
dnl autoconf versions before 2.63b don't have AS_VAR_APPEND
m4_ifdef([AS_VAR_APPEND],,[as_fn_append () { eval $[1]=\$$[1]\$[2]; }
AC_DEFUN([AS_VAR_APPEND],[as_fn_append $1 $2])])dnl
I assume we need something similar here, too? I am really far from
an autoconf hacker -- is this something one of you could look into?
Gerald
Re: wineps.drv: Remove variables scale_xx, scale_xy, scale_yx, and scale_yy and two dozen lines of dead code from append_complex_glyph.
On Sun, 9 May 2010, Nikolay Sivov wrote: I don't think it's dead. Variable ptr is used in a cycle. You are amazing. And right. I just submitted an updated patch. Gerald
Re: gdi32/tests: Remove variable oldPen which is not really used from test_widenpath.
On Sun, 9 May 2010, Vitaliy Margolen wrote: On 05/07/2010 01:06 PM, Gerald Pfeifer wrote: -oldPen = SelectObject(hdc, greenPen); +SelectObject(hdc, greenPen); If it's not used it's a bug. Everything should be reset to original state to prevent influence on following tests. There are number of such places in Wine's test suite that makes it impossible to isolate the problem. Hmm, the function test_widenpath starts with HDC hdc = GetDC(0) and ends with ReleaseDC(0, hdc). Do you suggest to SelectObject(hdc,oldPen) before the latter? Isn't that a no-op? There are two instances in that function where a pen is selected. Do you suggest to restore to the old pen before the second? Looking at the test, I'm not sure how relevant that would be. (Alexandre already committed my original patch, but I don't want to just ignore your feedback. It's well possible I may be missing something.) Gerald
Re: comctl32/tests: Remove variable res which is not really used from test_monthcal_size.
On Thu, 6 May 2010, Nikolay Sivov wrote: -res = SendMessage(hwnd, MCM_GETMINREQRECT, 0, (LPARAM)r1); +SendMessage(hwnd, MCM_GETMINREQRECT, 0, (LPARAM)r1); Actually it won't hurt to test for it here. Okay. I am currently testing a patch that does exactly that in both cases. Thanks for the constructive feedback! Gerald
Re: advpack: Only do_ocx_reg (and thus DllRegisterServer) from RegisterOCX when 'N' is passed as a flag. Clarify documentation.
On Tue, 11 May 2010, James Hawkins wrote: I'm very hesitant about this. MSDN has no documentation about RegisterOCX, so I'm not sure how you're justifying this change. It's been a long time since I worked on this, so I don't remember much, but I do remember testing this method and documenting the parameters correctly. Where are you getting information that 'I' is required when using 'N'? I had to look again, and it seems the best I managed to find is the following: http://msdn.microsoft.com/en-us/library/bb759846%28VS.85%29.aspx If you experimentally verified something differently I will be happy to follow that. However, I did not found the original documentation sufficiently clear to really use it as a specification to base the implementation on. Specific questions I ran into were: - what happens if none of these are specified? - can the string contain more than one character? (pressumably yes?) - what happens if both are specified? Would you feel more comfortable leaving the documentation as is and me just suggesting the following? if(strchrW(str_flags,'I')) hr = do_ocx_reg(hm, TRUE); to replace hr = do_ocx_reg(hm, TRUE); ? Or would you prefer to submit a patch to clarify the documentation (copying me) and based on that I hack the code? That way we'd ensure that the former is sufficiently clear and I assume you'll verify whether the code matches that? Whatever works for you as long as we make progress. :-) Gerald
Re: Remove variable hFont which is not really used from MF_PlayMetaFile. (RESEND)
On Mon, 10 May 2010, Alexandre Julliard wrote: dlls/gdi32/metafile.c |2 -- 1 files changed, 0 insertions(+), 2 deletions(-) It seems pretty clear that it should be used, not removed. Eeeh. That one's worth a drink the next (= first) time we meet in person. (On the positive side, at least my work _is_ unearthing some bugs. :-) I'll momentarily submit a proper patch. Gerald
Re: kernel32/tests: Remove variable ret which is not really used from load_blackbox. (RESEND)
On Sun, 9 May 2010, test...@testbot.winehq.org wrote: While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=1969 I did double check, and this is a false positive. This failure is not (and really cannot) be caused by my patch. Is the patch acceptable with that in mind? Gerald
Re: user32/tests: Remove SetShellWindowEx from test_shell_window.
On Sun, 9 May 2010, test...@testbot.winehq.org wrote: While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=1974 That really cannot be related to my test (unless there is some most subtle timing issue involved, perhaps, which is completel unrelated). It appears as if these tests all run into problems on the W7 flavor, regardless and independently of my patch. Gerald
Re: user32/tests: Remove variable atom which is not really used from test_Expose.
On Sun, 9 May 2010, test...@testbot.winehq.org wrote: Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=1983 Another false positive. W7PROcompleted 39 W7PROX64 completed 19 W7PROX64 completed 19 ...is the same before and after this patch of mine. Gerald
Error handling in RpcBindingVectorFree (dlls/rpcrt4/rpc_binding.c)
Looking at
RPC_STATUS WINAPI RpcBindingVectorFree( RPC_BINDING_VECTOR** BindingVector )
{
RPC_STATUS status;
ULONG c;
TRACE((%p)\n, BindingVector);
for (c=0; c(*BindingVector)-Count; c++) {
status = RpcBindingFree((*BindingVector)-BindingH[c]);
}
HeapFree(GetProcessHeap(), 0, *BindingVector);
*BindingVector = NULL;
return RPC_S_OK;
}
we currently always ignore the outcome of RpcBindingFree and return
RPC_S_OK.
However, there is one case where RpcBindingFree returns something
different (which is if *Binding is null when RPC_S_INVALID_BINDING
is returned).
What is the proper way of handling this? Just keeping the code as
is and removing the unused status variable? Breaking the loop once
RpcBindingFree returns something different from RPC_S_OK? Continuing
and returning the first / the last status different from RPC_S_OK?
Gerald
Re: localspl/tests: Improve the tests in test_XcvDataPort_AddPort by properly checking return values and avoiding a duplicat
On Sun, 9 May 2010, test...@testbot.winehq.org wrote: === WINEBUILD (build) === Patch failed The issue was that a previous patch of mine (which got accepted) conflicted and my local tree was not right. Fixed and resubmitted, testing at Wine Test Bot went well now: https://testbot.winehq.org/JobDetails.pl?Key=1975 Gerald
Re: mshtml/tests: Fix return value of ActiveScript_SetScriptState.
On Mon, 3 May 2010, Jacek Caban wrote: On 5/2/10 9:16 PM, Gerald Pfeifer wrote: IActiveScriptSite_OnStateChange is described to return S_OK upon success, so instead of ignoring its return value and unconditionally returning S_OK it strikes me that we should return its result instead. In this case returning S_OK is fine, but there is missing test for OnStateChange return value. Hmm, but if you look at the patch, in those cases we call OnStateChange, we actually pass on it's return value: hres = IActiveScriptSite_OnStateChange(site, (state = ss)); return hres; And that is S_OK if successful according to MSDN. Perhaps you could follow up with a patch to refine what we have now? Gerald
Re: gdi32/tests: Remove two variables which are not really used from test_clipping.
On Mon, 3 May 2010, Nikolay Sivov wrote: Bitmap are useless after this change. I'll submit a patch which has this fixed. Interestingly, at least on my FreeBSD-based tester, test results did _not_ regress either way. Gerald
Re: kernel32/tests: Remove variable len which is not really used from test_CommandLine.
On Wed, 5 May 2010, test...@testbot.winehq.org wrote: While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=1901 Your paranoid android. === W98SE (32 bit process) === Failure running script in VM: Exceeded timeout limit of 315 sec There is really no way this patch can have caused this test failure. So, I double-checked. :-) What is the proper way to remark such a false positive? Follow up here, on wine-devel as I am doing now? Retrigger the test somehow? Gerald PS: I'm sorry if I missed something on the Wiki, I did check for it.
Re: gdi32/tests: Remove two variables which are not really used from test_clipping.
On Mon, 3 May 2010, Nikolay Sivov wrote: Bitmap are useless after this change. Darn, I may have sent the wrong version of the patch. Let me look into this tonight or tomorrow. You have sharp eyes, Nikolay! :-) Gerald
Re: Minor dlls/dbghelp/dwarf.c simplification
Eric Pouech wrote:
this is wrong: dwarf_parse family set makes the pointer in the cxt
advance by the size of the object which is being parse
You're right. I had, in fact, checked this very question, but doing
so just again I see where I missed the += in the code. Thanks for
catching this, Eric!
and actually, the correct fix would be to make use of those variables
(like checking if the version is a known one), instead of throwing
things away
Are you planning to go into that direction? If not, I'd suggest the
patch below. (I'm not sure whether checking for the DWARF version is
going to help -- do we want to abort for newer versions when these
actually should be compatible?)
Gerald
From: Gerald Pfeifer ger...@pfeifer.com
Date: Sun, 2 May 2010 22:05:20
Subject: dbghelp: Remove two variables which are not really used in
dwarf2_parse_line_numbers.
---
dlls/dbghelp/dwarf.c |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/dlls/dbghelp/dwarf.c b/dlls/dbghelp/dwarf.c
index 4be0f6a..99960d8 100644
--- a/dlls/dbghelp/dwarf.c
+++ b/dlls/dbghelp/dwarf.c
@@ -1920,7 +1920,7 @@ static BOOL dwarf2_parse_line_numbers(const
dwarf2_section_t* sections,
{
dwarf2_traverse_context_t traverse;
unsigned long length;
-unsignedversion, header_len, insn_size, default_stmt;
+unsignedinsn_size, default_stmt;
unsignedline_range, opcode_base;
int line_base;
const unsigned char*opcode_len;
@@ -1939,8 +1939,8 @@ static BOOL dwarf2_parse_line_numbers(const
dwarf2_section_t* sections,
length = dwarf2_parse_u4(traverse);
traverse.end_data = sections[section_line].address + offset + length;
-version = dwarf2_parse_u2(traverse);
-header_len = dwarf2_parse_u4(traverse);
+dwarf2_parse_u2(traverse);
+dwarf2_parse_u4(traverse);
insn_size = dwarf2_parse_byte(traverse);
default_stmt = dwarf2_parse_byte(traverse);
line_base = (signed char)dwarf2_parse_byte(traverse);
--
1.6.6.2
Re: Remove variable doRecursive which is not really used from WCMD_for.
On Sun, 2 May 2010, Nikolay Sivov wrote: Please use consistent naming for patches and mail subjects. It's described in http://wiki.winehq.org/SubmittingPatches. I'm not sure it makes sense to resend all these, but GERALD: as a start word is unacceptable of course. Indeed, four or so patches with that in the Subject sneaked through. This simply was a mistake on my side last month (and Alexandre was so kind to fix it up). As for prepending what basically seems to be `pwd | xargs basename` (but not exactly), I updated my script to take that into account. Thanks for the advise! Gerald PS: It's somewhat funny that after contributing for elven years, the first time someone complains about my mail subjects is a week or two _after_ I switch over to using git format-patch. ;-)
Re: Minor dlls/comctl32/tests/listview.c simplification
On Thu, 22 Apr 2010, Nikolay Sivov wrote: You can't do this, subclassing is important thing here. And you will see it if you run a test (that you should do before sending patch actually). Thanks, Nikolay. You're perfectly right, that was a silly mistake of mine, no way to put this differently. I've had problems running the tests on my machine I primarily use for Wine work, but now managed to address this (which involved patching mmap.c, but seems to work now). Gerald
Re: Minor dlls/localspl/tests/localmon.c simplificiation
On Sat, 1 May 2010, Nikolay Sivov wrote:
ChangeLog:
Remove variable res which is not really used from test_XcvClosePort.
We should better test for return value than remove it.
This is a primitive thing, try it if you didn't before.
All you need is to add some lines like:
---
ok(res == ERROR_SUCCESS /*a proper code here*/, expected ERROR_SUCCESS, got
%d\n, res);
---
After that run a test on Windows and Wine (using a bot for example) and make
corrections if needed.
Aaactually, now that I was going to look into this, I recall why I didn't
consider this originally: The entire code in that function is inside an
if (0) { }... :-o
But, since you asked for it, I added such tests across the board, made one
simplification, verified that it actually (if enabled) passes properly on
Wine and will submit this in a minute. Then we have things in place when
the FIXME is addressed. Plus a learned a thing or two in the process. ;-)
Thanks for your continous feedbac, Nikolay!
Gerlald
Re: Minor dlls/localspl/tests/localmon.c simplificiation
On Sat, 24 Apr 2010, Nikolay Sivov wrote: ChangeLog: Remove variable res which is not really used from test_XcvClosePort. We should better test for return value than remove it. Okay. Is this something you can look into (or do you have a hint on how you'd like such a test to look like)? It would be great could you make this change, but if you have guidance I will do my best to give it a try, too. Gerald
Wine without X? Failing in dlls/winex11.drv
Trying to build Wine on a new tester which initially had too few packages installed I ran into the following. In file included from bitblt.c:33: x11drv.h:30:22: error: X11/Xlib.h: No such file or directory x11drv.h:31:27: error: X11/Xresource.h: No such file or directory x11drv.h:32:23: error: X11/Xutil.h: No such file or directory x11drv.h:33:23: error: X11/Xatom.h: No such file or directory x11drv.h:44:21: error: X11/Xmd.h: No such file or directory x11drv.h:45:24: error: X11/Xproto.h: No such file or directory Indeed include/config.h has HAVE_X11_XLIB_H undefined since, well, all these headers indeed are missing. On the other hand, configure did not halt. In fact, configure did not even warn about this situation unlike it does in other cases. How to fix this? Should dlls/winex11.drv just not be built in such a case? Should configure error out? (We do have --without-x, too, though.) Anything else? Currently we do pass configure and then run into a compile failure later on. Gerald
Re: Student Interested in Google Summer of Code 2010
On Wed, 27 Jan 2010, Tom Wickline wrote: The version check fails on FreeBSD 8 even tho flex-2.5.35_3 is installed from ports. # flex --version flex version 2.5.4 The version check passes on Linux and OpenSolaris but FreeBSD has a harder time with it. Gerald has a workaround for this.. Shall I call it a bug? since a newer version is actually installed but the version check fails. It's not a bug in Wine or Wine's configure scripts. What happens on your system, or any standard FreeBSD system with the flex package installed is that the default path has /usr/bin before /usr/local/bin, so /usr/bin/flex which is 2.5.4 (as per your example) is found before, or should I say: instead, /usr/local/bin/flex which is the good one. But if you download the Wine source and try to compile your out of luck unless you remove the version check from configure, which is what Ive done as a temporary fix. Real fixes are setting your path to take /usr/local/bin before /usr/bin or probably better setting FLEX=/usr/local/bin/flex in your environment which tells configure and other tools to use that version of flex instead of the one in the base system. I would not recommend Wine trying to second guess which of several versions of flex on a system to use as opposed to following the common standard of use the first instance in the search path. I hope this makes sense? Gerald -- Gerald (Jerry) Pfeifer ger...@pfeifer.com http://www.pfeifer.com/gerald/
Re: Student Interested in Google Summer of Code 2010
On Fri, 29 Jan 2010, Austin English wrote: Right. But it's a bug in the sense that FreeBSD is shipping an _extremely_ old version of flex ;-). True. Can you put in a request to update flex for FreeBSD 8.1/9.0? Or should I register/file a bug? Filing a request is fine, if you want to do so. Just don't hold your breath, there is a lot of FUD around GPLv3 in BSD-land¹ and anything with a GPLv3 license in the base system will need core (team) approval and a heck of a lot of a reason. Gerald ¹ Personally I do agree that GPLv3 was not very helpful, alas it's not the end of the world, either.
Re: Student Interested in Google Summer of Code 2010
On Fri, 29 Jan 2010, Austin English wrote: Flex isn't GLPv3, it's got a BSD license: http://flex.sourceforge.net/manual/Copyright.html#Copyright You're right, and I had actually checked that a few weeks ago and confused it with something else now. Excellent. Please do file a PR against FreeBSD and let me know the number off-list. I may have an idea... :-) Gerald
Re: Use GCC's -Wlogical-op if possible
On Tue, 23 Jun 2009, Paul Vriens wrote: For those interested, here's the make log in Monday's git. 94 warnings for me. Sent 3 patches based on your log. I have less (55) warnings on 4.3.2 btw, but most look like false positives. On my FreeBSD test system I am see no warnings triggered by -Wlogical-op any more. How does it look on your side? Depending on your findings, we may want to enable -Wlogical-op by default and I'd submit a patch to that end. Gerald
Re: Use GCC's -Wlogical-op if possible
On Sun, 3 Jan 2010, Austin English wrote: On my FreeBSD test system I am see no warnings triggered by -Wlogical-op any more. How does it look on your side? ole32: usrmarshal.c:485: warning: logical ?? with non-zero constant will always evaluate as true usrmarshal.c:1098: warning: logical ?? with non-zero constant will always evaluate as true usrmarshal.c:1290: warning: logical ?? with non-zero constant will always evaluate as true oleaut32: variant.c:2090: warning: logical ?||? with non-zero constant will always evaluate as true variant.c:2090: warning: logical ?? with non-zero constant will always evaluate as true comctl32/tests: tab.c:520: warning: logical ?? with non-zero constant will always evaluate as true tab.c:540: warning: logical ?? with non-zero constant will always evaluate as true tab.c:563: warning: logical ?? with non-zero constant will always evaluate as true tab.c:978: warning: logical ?? with non-zero constant will always evaluate as true I had a patch for this one (comctl32/tests) which I received feedback on and need to brush up. I should be able to do so coming week. Anybody volunteering to look into the other ones? kernel32/tests: atom.c:70: warning: logical ?||? with non-zero constant will always evaluate as true Gerald
va_list breakage introduced today
I believe the following patch commit 8268ed978389662c7b43212e008037cae3ce900e Author: Alexandre Julliard julli...@winehq.org Date: Mon Dec 28 22:19:31 2009 +0100 kernel32: Do not include 16-bit headers in 32-bit files. is causing gmake[2]: Entering directory `/usr/test/Wine/dlls/kernel32' /files/pfeifer/gcc/bin/gcc -c -I. -I. -I../../include -I../../include -D__WINESRC__ -D_KERNEL32_ -D_REENTRANT -fPIC -Wall -pipe -fno-strict-aliasing -Wdeclaration-after-statement -Wstrict-prototypes -Wwrite-strings -Wtype-limits -Wpointer-arith -I/usr/local/include -g -O2 -o module.o module.c In file included from module.c:36:0: ../../include/winbase.h:1560:84: error: expected declaration specifiers or '...' before 'va_list' ../../include/winbase.h:1561:85: error: expected declaration specifiers or '...' before 'va_list' In file included from module.c:37:0: ../../include/winternl.h:2223:81: error: expected declaration specifiers or '...' before 'va_list' ../../include/winternl.h:2389:58: error: expected declaration specifiers or '...' before 'va_list' ../../include/winternl.h:2390:75: error: expected declaration specifiers or '...' before 'va_list' over here on my FreeBSD 7.2 tester. Gerald
Re: va_list breakage introduced today
On Thu, 31 Dec 2009, Reece Dunn wrote: Have you tried this patch?: http://www.winehq.org/pipermail/wine-patches/2009-December/083364.html (This is in the latest git -- Yep, that works. Thanks! Gerald
Re: Remove cpp_quote hackery from wined3d.idl
On Thu, 24 Dec 2009, Henri Verbeet wrote:
Done thusly. We just need to move WINEMAKEFOURCC to a local header,
the rest of the cpp_quote can go and we lose a couple of dozen warning
messages (rightfully) issued by GCC 4.5 snapshots.
I already sent pretty much the same patch,
http://source.winehq.org/git/wine.git/?a=commitdiff;h=3288911ae3e3cbd7124ed60d805ff3310f6a21c9
Ugh. My experience with Wine always has been that if an expert in some
area like you suggests an approach, I'd be the one expected to implement
that. Thanks for taking care of it this time, though getting a heads up
would have been nice (and would have avoided duplicate efforts).
That said, do you have an idea on how to best tackle the two remaining
cases?
In dlls/wined3d/directx.c we have the following where I wonder whether
you may want to add this to the enum as well?
case WINEMAKEFOURCC('I','N','S','T'):
TRACE(ATI Instancing check hack\n);
if (gl_info-supported[ARB_VERTEX_PROGRAM] ||
gl_info-supported[ARB_VERTEX_SHADER])
{
TRACE_(d3d_caps)([OK]\n);
return TRUE;
}
TRACE_(d3d_caps)([FAILED]\n);
return FALSE;
And in dlls/wined3d/utils.c we have TSTYPE_TO_STR(WINED3DTS_WORLDMATRIX(0))
in debug_d3dtstype which also triggers the warning.
Thanks,
Gerald
Re: tools/widl: Split expr_int_const off from expr
Hi Rob, first of all sorry for the delay in getting back to this. I
kept looking for alternative approaches, but now ended up figuring that
really laying down the issue at hand I was trying to solve in front of
everyone a priori will be the better approach.
Let me first respond to concrete feedback you provided, and than show
why I started down this road.
On Thu, 2 Jul 2009, Rob Shearman wrote:
2009/6/28 Gerald Pfeifer ger...@pfeifer.com:
diff --git a/tools/widl/parser.y b/tools/widl/parser.y
index c2f1abc..01aa060 100644
--- a/tools/widl/parser.y
+++ b/tools/widl/parser.y
@@ -622,56 +633,54 @@ m_expr: { $$
= make_expr(EXPR_VOID); }
| expr
;
-expr: aNUM { $$ = make_exprl(EXPR_NUM,
$1); }
- | aHEXNUM { $$ =
make_exprl(EXPR_HEXNUM, $1); }
+expr: expr_int_const
| aDOUBLE { $$ =
make_exprd(EXPR_DOUBLE, $1); }
+ | '-' aDOUBLE { $$ =
make_exprd(EXPR_DOUBLE, -($2)); }
This is covered by the production for the binary minus operator that
already exists, so I'm not sure what you're trying to achieve here.
Yes, alas the current widl parser is somewhat special: it uses quite
general rules (too general rules, one could argue), only to perform
tighter checks later on, in the C component of the productions.
This basically combines the styles of bison parser with a recursive
descending parser (manually written), and I tried to clear this a bit.
Using expr_int_const instead of expr here prevents a many forms of
expressions from being parsed.
It did suffice to build all of Wine without problems, but if we expect
out of tree users, that could be a problem indeed.
It looks like you'll have to find another way of fixing the issue you
are trying to fix.
Okay, so here is the issue. Wine kind of abuses enums a bit, injecting
additional values by means of #defines (that are not part of the enum
proper) which will make GCC 4.5, without extra options!, issue the
following litany of warnings:
utils.c:269:9: warning: case value '827606349' not in enumerated type
'WINED3DFORMAT'
utils.c:264:9: warning: case value '827611204' not in enumerated type
'WINED3DFORMAT'
utils.c:258:9: warning: case value '842094169' not in enumerated type
'WINED3DFORMAT'
utils.c:265:9: warning: case value '844388420' not in enumerated type
'WINED3DFORMAT'
utils.c:252:9: warning: case value '844715353' not in enumerated type
'WINED3DFORMAT'
utils.c:266:9: warning: case value '861165636' not in enumerated type
'WINED3DFORMAT'
utils.c:267:9: warning: case value '877942852' not in enumerated type
'WINED3DFORMAT'
utils.c:268:9: warning: case value '894720068' not in enumerated type
'WINED3DFORMAT'
utils.c:270:9: warning: case value '970375' not in enumerated type
'WINED3DFORMAT'
utils.c:271:9: warning: case value '1195525970' not in enumerated type
'WINED3DFORMAT'
utils.c:251:9: warning: case value '1498831189' not in enumerated type
'WINED3DFORMAT'
directx.c:2863:9: warning: case value '827611204' not in enumerated type
'WINED3DFORMAT'
directx.c:2864:9: warning: case value '844388420' not in enumerated type
'WINED3DFORMAT'
directx.c:2865:9: warning: case value '861165636' not in enumerated type
'WINED3DFORMAT'
directx.c:2866:9: warning: case value '877942852' not in enumerated type
'WINED3DFORMAT'
directx.c:2867:9: warning: case value '894720068' not in enumerated type
'WINED3DFORMAT'
directx.c:3116:9: warning: case value '827606349' not in enumerated type
'WINED3DFORMAT'
directx.c:3017:9: warning: case value '827611204' not in enumerated type
'WINED3DFORMAT'
directx.c:3060:9: warning: case value '842094169' not in enumerated type
'WINED3DFORMAT'
directx.c:3121:9: warning: case value '843666497' not in enumerated type
'WINED3DFORMAT'
directx.c:3018:9: warning: case value '844388420' not in enumerated type
'WINED3DFORMAT'
directx.c:3052:9: warning: case value '844715353' not in enumerated type
'WINED3DFORMAT'
directx.c:3019:9: warning: case value '861165636' not in enumerated type
'WINED3DFORMAT'
directx.c:3020:9: warning: case value '877942852' not in enumerated type
'WINED3DFORMAT'
directx.c:3021:9: warning: case value '894720068' not in enumerated type
'WINED3DFORMAT'
directx.c:3115:9: warning: case value '970375' not in enumerated type
'WINED3DFORMAT'
directx.c:3114:9: warning: case value '1195525970' not in enumerated type
'WINED3DFORMAT'
directx.c:3141:9: warning: case value '1397249614' not in enumerated type
'WINED3DFORMAT'
directx.c:3103:9: warning: case value '1414745673' not in enumerated type
'WINED3DFORMAT'
directx.c:3140:9: warning: case value '1430804046' not in enumerated type
'WINED3DFORMAT'
directx.c:3051:9: warning: case value '1498831189' not in enumerated type
'WINED3DFORMAT'
directx.c:3709:13: warning: case value '827611204' not in enumerated type
'WINED3DFORMAT
Re: tools/winebuild: add FreeBSD support (1/2) (RESEND) (fwd)
On Tue, 6 Oct 2009, Alexandre Julliard wrote:
If you prefer, I'll be happy to convert the code into a switch statement.
Just let me know!
That would certainly be more elegant.
Your wish is my command. :-)
Gerald
ChangeLog:
Add support for PLATFORM_FREEBSD to get_ld_command.
diff --git a/tools/winebuild/utils.c b/tools/winebuild/utils.c
index e0cd8bc..6008ef4 100644
--- a/tools/winebuild/utils.c
+++ b/tools/winebuild/utils.c
@@ -297,9 +297,23 @@ const char *get_ld_command(void)
if (force_pointer_size)
{
-const char *args = (target_platform == PLATFORM_APPLE) ?
-((force_pointer_size == 8) ? -arch x86_64 : -arch i386) :
-((force_pointer_size == 8) ? -m elf_x86_64 : -m
elf_i386);
+const char *args;
+
+switch (target_platform)
+{
+case PLATFORM_APPLE:
+args = (force_pointer_size == 8) ? -arch x86_64
+ : -arch i386;
+break;
+case PLATFORM_FREEBSD:
+args = (force_pointer_size == 8) ? -m elf_x86_64
+ : -m elf_i386_fbsd;
+break;
+default:
+args = (force_pointer_size == 8) ? -m elf_x86_64
+ : -m elf_i386;
+}
+
ld_command = xrealloc( ld_command, strlen(ld_command) +
strlen(args) + 1 );
strcat( ld_command, args );
}
Re: Minor fix for include/bits1_5.idl
Strike that, I must have misread the documentation. Only thing I am wondering is do we really need the (unsigned long) here? If anyone has a pointer to clear documentation that would be nice; what I found so far leaves some questions open... Gerald On Sun, 28 Jun 2009, Gerald Pfeifer wrote: The generated file include/bits1_5.h is the same before and after this patch, yet somehow the original form strikes me as a bit odd at best... Gerald ChangeLog: Fix prototype for GetReplyData(). diff --git a/include/bits1_5.idl b/include/bits1_5.idl index 274a7de..29520e2 100644 --- a/include/bits1_5.idl +++ b/include/bits1_5.idl @@ -39,7 +39,7 @@ interface IBackgroundCopyJob2 : IBackgroundCopyJob } BG_JOB_REPLY_PROGRESS; HRESULT GetReplyProgress([in, out] BG_JOB_REPLY_PROGRESS *progress); -HRESULT GetReplyData([out, size_is( , (unsigned long) *pLength)] byte **pBuffer, +HRESULT GetReplyData([out, size_is((unsigned long) *pLength)] byte **pBuffer, [in, out, unique] UINT64 *pLength); HRESULT SetReplyFileName([unique] LPCWSTR filename); HRESULT GetReplyFileName([out] LPWSTR *pFilename);
Re: Fix error checking in dlls/ddraw/executebuffer.c
On Sat, 20 Jun 2009, Paul Vriens wrote: - if (!ci-u1.dlstLightStateType (ci-u1.dlstLightStateType D3DLIGHTSTATE_COLORVERTEX)) +if (!ci-u1.dlstLightStateType || (ci-u1.dlstLightStateType D3DLIGHTSTATE_COLORVERTEX)) Would: if ((ci-u1.dlstLightStateType D3DLIGHTSTATE_MATERIAL) || (ci-u1.dlstLightStateType D3DLIGHTSTATE_COLORVERTEX)) be easier to read? (Matter of taste I guess). I found the existing check (with the bug fixed ;-) easier to understand, but as you say it's a matter of taste and I do not feel strongly about it. Gerald
Re: Use GCC's -Wlogical-op if possible
On Fri, 19 Jun 2009, Ben Klein wrote: diff --git a/configure.ac b/configure.ac index bef311e..3f7a657 100644 --- a/configure.ac +++ b/configure.ac @@ -1385,8 +1385,9 @@ then WINE_TRY_CFLAGS([-fno-builtin],[AC_SUBST(BUILTINFLAG,-fno-builtin)]) WINE_TRY_CFLAGS([-fno-strict-aliasing]) WINE_TRY_CFLAGS([-Wdeclaration-after-statement]) - WINE_TRY_CFLAGS([-Wwrite-strings]) + WINE_TRY_CFLAGS([-Wlogical-op]) WINE_TRY_CFLAGS([-Wtype-limits]) + WINE_TRY_CFLAGS([-Wwrite-strings]) Is there a reason why -Wwrite-strings is moved? Alphabetical sorting, now the list gets longer. :-) Gerald
Re: Use GCC's -Wlogical-op if possible
On Thu, 18 Jun 2009, Austin English wrote: Causes 106 more warnings on 4.3.3 of this sort: tab.c:693: warning: logical ?? with non-zero constant will always evaluate as true cert.c:1627: warning: logical ?||? with non-zero constant will always evaluate as true This is strange. In that case, let's hold off and I will work on addressing these first (testing with even more versions of GCC than I had done). Gerald
Re: Use GCC's -Wlogical-op if possible
On Fri, 19 Jun 2009, Rein Klazes wrote: cert.c:1627: warning: logical ?||? with non-zero constant will always evaluate as true That (in dlls/crypt32/tests) looks like a real bug to me. Indeed. And Alexandre committed a fix of mine in the last 24 hours. ;-) Gerald
Re: Use GCC's -Wlogical-op if possible
On Fri, 19 Jun 2009, Paul Vriens wrote: I have about 70 warnings with 4.3.2. I already sent two patches and found a few more bugs. If you want I can start sending in patches for those as well (don't want to duplicate work of course). Thanks for checking to help avoid duplicate work, Paul; I really appreciate that. Austin was kind enough to share his logs, and I now submitted patches for (more or less) everything I had seen on my side and could reproduce based on this input. If you want to go for the rest you are seeing after those few patches of mine, that would be great. If you could keep us updated on how it goes (ideally bringing warnings down to zero, of course ;-) that would be nice, too. Happy hacking! :) Gerald
dlls/winedos/int10.c
In dlls/winedos/int10.c we currently have the following code VGA_SetBright((BL_reg(context) 0x10) 1); which I am not sure I understand. Is the purpose of this to only pass one of 0 or 1 to VGA_SetBright? If so, I guess I'd find ... ? 1 : 0 more intuitive, and it's only three characters more, or != 0 at least. Thoughts? Gerald
Re: C_ASSERT portability fixes and simplification [PATCH 1/2]
On Mon, 11 May 2009, Alexandre Julliard wrote:
I don't have gcc 4.5 to test this, but you could try using a function
instead, something like this:
diff --git a/include/winnt.h b/include/winnt.h
index abcc502..aaa4112 100644
--- a/include/winnt.h
+++ b/include/winnt.h
@@ -292,7 +292,7 @@ extern C {
#if defined(_MSC_VER)
# define C_ASSERT(e) typedef char __C_ASSERT__[(e)?1:-1]
#elif defined(__GNUC__)
-# define C_ASSERT(e) extern char __C_ASSERT__[(e)?1:-1]
__attribute__((unused))
+# define C_ASSERT(e) extern void __C_ASSERT__(int [(e)?1:-1])
#else
# define C_ASSERT(e)
#endif
You are a clever man, Sir! :-)
It took me a bit, since incidently the GCC 4.5 development tree was
broken wrt. Wine on Monday (I reported this bug upstream), but a
build with your patch went through both with GCC 4.2.1 and GCC 4.5.0
(experimental) tonight.
I assume you will commit this now?
Thanks,
Gerald
Re: C_ASSERT portability fixes and simplification [PATCH 1/2]
On Mon, 11 May 2009, Alexandre Julliard wrote: 2. More importantly, these were the only occurences of C_ASSERT at file level. By moving these at function level use thereof becomes consistent which will allow the fix from PATCH 2/2 to work at all. C_ASSERT is supposed to work at the file level too. The current implementation doesn't work with GCC 4.5 (or ISO C) and I failed to find any other way. :-( Your point on this being autogenerated clearly is strong one -- either we need to change that generation or find a different way to implement C_ASSERT. Any idea how to best approach this? Gerald
Re: Simplify dlls/comctl32/rebar.c
On Mon, 2 Feb 2009, Francois Gouget wrote:
/* case RB_SETCOLORSCHEME: */
/* case RB_SETPALETTE: */
-/* return REBAR_GetPalette (infoPtr, wParam, lParam); */
case RB_SETPARENT:
Once the commented out return has been removed it looks like
RB_SETCOLORSCHEME and RB_SETPALETTE are meant to fall through to
RB_SETPARENT, which does not seem right...
So I'd leave a comment there, maybe /* return ...; */.
That is a very fair point, thanks for raising this Francois!
Let me adjust my patch accordingly.
Gerald
ChangeLog:
Simplify.
Index: dlls/comctl32/rebar.c
===
RCS file: /home/wine/wine/dlls/comctl32/rebar.c,v
retrieving revision 1.166
diff -u -3 -p -r1.166 rebar.c
--- dlls/comctl32/rebar.c 21 Nov 2008 12:56:20 - 1.166
+++ dlls/comctl32/rebar.c 3 Feb 2009 00:08:59 -
@@ -2255,7 +2255,7 @@ REBAR_GetBkColor (const REBAR_INFO *info
static LRESULT
-REBAR_GetPalette (const REBAR_INFO *infoPtr, WPARAM wParam, LPARAM lParam)
+REBAR_GetPalette ()
{
FIXME(empty stub!\n);
@@ -3557,7 +3557,7 @@ REBAR_WindowProc (HWND hwnd, UINT uMsg,
/* case RB_GETDROPTARGET: */
case RB_GETPALETTE:
- return REBAR_GetPalette (infoPtr, wParam, lParam);
+ return REBAR_GetPalette ();
case RB_GETRECT:
return REBAR_GetRect (infoPtr, wParam, lParam);
@@ -3618,7 +3618,7 @@ REBAR_WindowProc (HWND hwnd, UINT uMsg,
/* case RB_SETCOLORSCHEME: */
/* case RB_SETPALETTE: */
-/* return REBAR_GetPalette (infoPtr, wParam, lParam); */
+/* return REBAR_GetPalette (); */
case RB_SETPARENT:
return REBAR_SetParent (infoPtr, wParam);
Re: Adjust dlls/iphlpapi/ipstats.c to FreeBSD 8
On Fri, 9 Jan 2009, Gerald Pfeifer wrote:
I am not aware of one. Tijl and me actually argued to get the
original behavior back (for this and other reasons like source
compatbility) but failed. I just pushed again.
FreeBSD upstream is not going to change, from what I can tell, so we
really need something like the following patch.
If you guys prefer something like
#ifndef RTF_LLINFO
#define RTF_LLINFO 0
#endif
instead, I can also create a patch for that!
Gerald
original message
From: Gerald Pfeifer ger...@pfeifer.com
To: wine-patc...@winehq.org
Date: Thu, 1 Jan 2009 20:32:51
Subject: Adjust dlls/iphlpapi/ipstats.c to FreeBSD 8
FreeBSD 8 is seeing a major rewrite of the arp code which removed
the RTF_LLINFO flag and thus broke Wine, among others, see
http://lists.freebsd.org/pipermail/freebsd-net/2008-December/020464.html
and the follow-ups for details.
The patch below is the solution for Wine (and other ports) agreed upon
by the respective FreeBSD core maintainers; a variant thereof has been
explicitly reviewed and approved.
Gerald
ChangeLog:
Only use RTF_LLINFO if #defined, fixing FreeBSD 8 after the arp-v2
rewrite.
Index: dlls/iphlpapi/ipstats.c
===
RCS file: /home/wine/wine/dlls/iphlpapi/ipstats.c,v
retrieving revision 1.37
diff -u -3 -p -r1.37 ipstats.c
--- dlls/iphlpapi/ipstats.c 30 Jun 2008 13:28:42 - 1.37
+++ dlls/iphlpapi/ipstats.c 1 Jan 2009 19:24:21 -
@@ -1250,7 +1250,13 @@ DWORD getRouteTable(PMIB_IPFORWARDTABLE
DWORD getNumArpEntries(void)
{
#if defined(HAVE_SYS_SYSCTL_H) defined(NET_RT_DUMP)
- int mib[] = {CTL_NET, PF_ROUTE, 0, AF_INET, NET_RT_FLAGS, RTF_LLINFO};
+ int mib[] = {CTL_NET, PF_ROUTE, 0, AF_INET, NET_RT_FLAGS,
+#ifdef RTF_LLINFO
+ RTF_LLINFO
+#else
+ 0
+#endif
+ };
#define MIB_LEN (sizeof(mib) / sizeof(mib[0]))
DWORD arpEntries = 0;
size_t needed;
@@ -1308,7 +1314,13 @@ DWORD getArpTable(PMIB_IPNETTABLE *ppIpN
#if defined(HAVE_SYS_SYSCTL_H) defined(NET_RT_DUMP)
if (table)
{
- int mib[] = {CTL_NET, PF_ROUTE, 0, AF_INET, NET_RT_FLAGS, RTF_LLINFO};
+ int mib[] = {CTL_NET, PF_ROUTE, 0, AF_INET, NET_RT_FLAGS,
+#ifdef RTF_LLINFO
+ RTF_LLINFO
+#else
+ 0
+#endif
+ };
#define MIB_LEN (sizeof(mib) / sizeof(mib[0]))
size_t needed;
char *buf, *lim, *next;
Re: Adjust dlls/iphlpapi/ipstats.c to FreeBSD 8
On Fri, 9 Jan 2009, Francois Gouget wrote:
On Thu, 1 Jan 2009, Gerald Pfeifer wrote:
[...]
ChangeLog:
Only use RTF_LLINFO if #defined, fixing FreeBSD 8 after the arp-v2
rewrite.
[...]
#if defined(HAVE_SYS_SYSCTL_H) defined(NET_RT_DUMP)
- int mib[] = {CTL_NET, PF_ROUTE, 0, AF_INET, NET_RT_FLAGS, RTF_LLINFO};
+ int mib[] = {CTL_NET, PF_ROUTE, 0, AF_INET, NET_RT_FLAGS,
+#ifdef RTF_LLINFO
+ RTF_LLINFO
+#else
+ 0
+#endif
+ };
Is there a way to do it so the same binary can run and work on FreeBSD
7.x and 8.0?
I am not aware of one. Tijl and me actually argued to get the
original behavior back (for this and other reasons like source
compatbility) but failed. I just pushed again.
(Well, one could try run-time checks instead of compile-time checks,
and then initialize mib[5] accordingly, but I have a hunch Alexandre
wouldn't like that very much...)
Gerald
Re: dlls/user32/dde_client.c warning fixes (RESEND)
On Wed, 31 Dec 2008, Alexandre Julliard wrote: Gerald Pfeifer ger...@pfeifer.com writes: The current version likely is actually broken since the ! operator has a higher priority than bitwise in C. Unless someone really wanted to use a very, hmm, unusual construct here? No, it's clearly not intended, but fixing this breaks the tests, this would need to be addressed first. Well, I found the bug and provided a fix, alas the testsuite is not exactly territory I feel home at. :-( I hope we won't keep the bug for the sake of not breaking a bogus test... Jeff, the vast majority of dlls/user32/tests/dde.c has been contributed by you -- is this something you could help with? Thanks, Gerald
RE: Simplify dlls/comctl32/nativefont.c
On Sat, 27 Dec 2008, ricardo filipe wrote: are you sure windows doesn't call those functions with those parameters? you have to check that before sending patches like these ... Yes, I am. Looking at the patch... static LRESULT -NATIVEFONT_Create (HWND hwnd, WPARAM wParam, LPARAM lParam) +NATIVEFONT_Create (HWND hwnd) ...you'll notice that this function is static and thus cannot be invoked by any external caller. I have explicitly excluded functions with external linkage or callback functions from such simplifications. Gerald -- Gerald (Jerry) Pfeifer ger...@pfeifer.com http://www.pfeifer.com/gerald/
implicit declaration of function '_mkdir'
I am mostly offline (2s ping times) for another two weeks, but noticed the following starting a couple of days ago on my nightly FreeBSD 6.4 tester: server.c:755: warning: implicit declaration of function '_mkdir' shellpath.c:2163: warning: implicit declaration of function '_mkdir' shfldr_unixfs.c:1745: warning: implicit declaration of function '_mkdir' xdg.c:253: warning: implicit declaration of function '_mkdir' theme.c:966: warning: implicit declaration of function '_mkdir' winemenubuilder.c:707: warning: implicit declaration of function '_mkdir' fd.c:1562: warning: implicit declaration of function '_mkdir' request.c:530: warning: implicit declaration of function '_mkdir' Perhaps one of you has an idea where this might come from and how to avoid it? Gerald
Re: Simplify dlls/atl/registrar.c
On Tue, 9 Sep 2008, Rob Shearman wrote: -static HRESULT Registrar_create(const IUnknown *pUnkOuter, REFIID riid, void **ppvObject) +static HRESULT Registrar_create(REFIID riid, void **ppvObject) A test needs to be added to see whether or not the Registrar class factory supports aggregation. If it does then a FIXME should be emitted. If not then an ERR may be emitted and CLASS_E_NOAGGREGATION returned. Just removing pUnkOuter isn't the right thing to do. Fair point! Sadly, this is beyond my expertise in this area and I hope that someone with that expertise is going to help with that? Is this something you could look at? (Based on your analysis consider my patch redrawn...) Gerald
Re: Unbreak dlls/winhttp on FreeBSD (and likely other BSDs/Darwin)
On Thu, 21 Aug 2008, Hans Leidekker wrote: Thanks. I didn't want to copy all of the ifdef blocks from wininet because I can't verify which ones are actually needed. So I'm counting on people like you to fix this up, and I'm sure I'll need your help again :) You're welcome. :-) Wine build have been surprisingly stable on FreeBSD the last two or so years, after being broken on a nearly weekly base a bit longer ago, so no worries. Gerald
Re: Remove unused variable in dlls/mlang/tests/mlang.c
On Thu, 5 Jun 2008, James Hawkins wrote: http://winehq.org/pipermail/wine-cvs/2008-June/044013.html I'm getting old and slow, I guess. :-/ Gerald
Re: Fix building tools/widl (for bison 1.75, among others)
On Fri, 2 May 2008, Robert Shearman wrote: It turns out that current versions of bison do not enforce the documented grammer as strictly as older ones such as bison 1.75. Fixed thusly. Oops, thanks for spotting it. Actually, I was developing on version 1.28 and didn't get those errors so it doesn't appear to be a deliberate change by the bison developers. This is interesting. Possibly the bison developers got feedback that the stricter checks in 1.75 caused to many troubles (even though they were correct per se)? Looks good. Please send to wine-patches. Oops. Originally I wasn't sure how to address the problem, so I started this e-mail to wine-devel and then failed to adjust the address once I had an actual patch. Thanks for spotting this -- the patch now made it into this week's release. :-) Gerald
Fix building tools/widl (for bison 1.75, among others)
After the following change about a week ago
Rob Shearman [EMAIL PROTECTED]
widl: Make the rules for parsing fields in structures, encapsulated unions
and non-encapsulated unions more strict.
Move the rules in fields that handle empty union cases into separate
union rules so that they can't erroneously be accepted for structures or
other types of unions.
I started seeing the following build failure on one older installation:
bison -p parser_ -o parser.tab.c -d parser.y
parser.y:749.2-751.15: type clash (`var' `attr_list') on default action
parser.y:751.16: parse error, unexpected :, expecting ; or |
parser.y:752.35-59: $2 of `ne_union_field' has no declared type
parser.y:757.2-759.7: type clash (`var' `') on default action
parser.y:759.8: parse error, unexpected :, expecting ; or |
parser.y:759.45-760.50: $1 of `union_field' has no declared type
parser.y:759.45-761.23: $2 of `union_field' has no declared type
gmake: *** [parser.tab.h] Error 1
It turns out that current versions of bison do not enforce the documented
grammer as strictly as older ones such as bison 1.75. Fixed thusly.
Gerald
ChangeLog:
Fix syntax to also work with older versions of bison.
Index: tools/widl/parser.y
===
RCS file: /home/wine/wine/tools/widl/parser.y,v
retrieving revision 1.199
diff -u -3 -p -r1.199 parser.y
--- tools/widl/parser.y 1 May 2008 18:38:07 - 1.199
+++ tools/widl/parser.y 2 May 2008 12:07:24 -
@@ -747,6 +747,7 @@ field:m_attributes decl_spec declarat
ne_union_field:
s_field ';' { $$ = $1; }
| attributes ';'{ $$ = make_var(NULL);
$$-attrs = $1; }
+;
ne_union_fields: { $$ = NULL; }
| ne_union_fields ne_union_field{ $$ = append_var( $1, $2 ); }
@@ -755,6 +756,7 @@ ne_union_fields:{ $$ = NULL; }
union_field:
s_field ';' { $$ = $1; }
| ';' { $$ = NULL; }
+;
s_field: m_attributes decl_spec declarator{ $$ = $3-var;
$$-attrs =
check_field_attrs($$-name, $1);
Re: dlls/fusion/assembly.c -- no-op error checks
On Wed, 16 Apr 2008, James Hawkins wrote: Beat ya to the punch :) Better twice than not at all, to paraphrase a German saying. :-) Gerald
Re: dlls/msi/streams.c type fixes
On Sun, 17 Feb 2008, James Hawkins wrote: static INT add_streams_to_table(MSISTREAMSVIEW *sv) +/* Return -1 in case of error. */ Don't add random comments like this. Okay, will keep this in mind, especially for dlls/msi. Gerald
Re: dlls/user32/edit.c -- remove superflous check (RESEND)
A second one which Marcus ended up reinventing, but at least the
underlying problem is solved now. :-/
Gerald
On Wed, 26 Dec 2007, Gerald Pfeifer wrote:
Okay. In that case, to account for possible future enhancements of
ImmGetCompositionStringW(), we'll need a patch like the following. The
current error handling code in EDIT_GetCompositionStr() simply doesn't
work due to the unsignedness of dwBufLen.
Gerald
ChangeLog:
Fix error handling in EDIT_GetCompositionStr().
Index: dlls/user32/edit.c
===
RCS file: /home/wine/wine/dlls/user32/edit.c,v
retrieving revision 1.16
diff -u -3 -p -r1.16 edit.c
--- dlls/user32/edit.c20 Nov 2007 16:56:17 - 1.16
+++ dlls/user32/edit.c26 Dec 2007 21:29:33 -
@@ -5360,6 +5348,7 @@ static void EDIT_UpdateText(EDITSTATE *e
static void EDIT_GetCompositionStr(HWND hwnd, LPARAM CompFlag, EDITSTATE *es)
{
+LONG l;
DWORD dwBufLen;
LPWSTR lpCompStr = NULL;
HIMC hIMC;
@@ -5369,9 +5358,9 @@ static void EDIT_GetCompositionStr(HWND
if (!(hIMC = ImmGetContext(hwnd)))
return;
-dwBufLen = ImmGetCompositionStringW(hIMC, GCS_COMPSTR, NULL, 0);
+dwBufLen = l = ImmGetCompositionStringW(hIMC, GCS_COMPSTR, NULL, 0);
-if (dwBufLen 0)
+if (l 0)
{
ImmReleaseContext(hwnd, hIMC);
return;
Re: dlls/msi/streams.c -- simplify and constify
For the record, while I don't think I saw a response to this, the
issue itself was now fixed with basically the same patch submitted
by Marcusso we can remove this from our radars.
Thanks for trying again, Marcus! :-)
Gerald
On Sat, 3 Nov 2007, Gerald Pfeifer wrote:
Good observation, though comparing an unsigned type against -1 is not
exactly good practice. Delving more into msi/streams.c, I came up with
the patch below which tries to address this (and some other issues).
If Alexandre and you prefer, I can change the check and really make it
just read sv-num_rows == -1. I just wanted to point out there also is
an alternative way.
Thanks again for your feedback. It was very helpful, and I'll watch
out for cases like this.
Gerald
Index: streams.c
===
RCS file: /home/wine/wine/dlls/msi/streams.c,v
retrieving revision 1.7
diff -u -3 -p -r1.7 streams.c
--- streams.c 18 Oct 2007 13:00:57 - 1.7
+++ streams.c 3 Nov 2007 22:01:13 -
@@ -39,7 +39,7 @@ WINE_DEFAULT_DEBUG_CHANNEL(msidb);
typedef struct tabSTREAM
{
-int str_index;
+UINT str_index;
LPWSTR name;
IStream *stream;
} STREAM;
@@ -54,7 +54,7 @@ typedef struct tagMSISTREAMSVIEW
UINT row_size;
} MSISTREAMSVIEW;
-static BOOL add_stream_to_table(MSISTREAMSVIEW *sv, STREAM *stream, int
index)
+static BOOL add_stream_to_table(MSISTREAMSVIEW *sv, STREAM *stream, UINT
index)
{
if (index = sv-max_streams)
{
@@ -315,7 +315,7 @@ static UINT STREAMS_modify(struct tagMSI
static UINT STREAMS_delete(struct tagMSIVIEW *view)
{
MSISTREAMSVIEW *sv = (MSISTREAMSVIEW *)view;
-int i;
+UINT i;
TRACE((%p)\n, view);
@@ -381,7 +381,8 @@ static const MSIVIEWOPS streams_ops =
NULL,
};
-static UINT add_streams_to_table(MSISTREAMSVIEW *sv)
+static int add_streams_to_table(MSISTREAMSVIEW *sv)
+/* Return -1 in case of error. */
{
IEnumSTATSTG *stgenum = NULL;
STATSTG stat;
@@ -433,6 +434,7 @@ static UINT add_streams_to_table(MSISTRE
UINT STREAMS_CreateView(MSIDATABASE *db, MSIVIEW **view)
{
MSISTREAMSVIEW *sv;
+INT i;
TRACE((%p, %p)\n, db, view);
@@ -442,10 +444,12 @@ UINT STREAMS_CreateView(MSIDATABASE *db,
sv-view.ops = streams_ops;
sv-db = db;
-sv-num_rows = add_streams_to_table(sv);
-if (sv-num_rows 0)
-return ERROR_FUNCTION_FAILED;
+i = add_streams_to_table(sv);
+if (i 0)
+ return ERROR_FUNCTION_FAILED;
+else
+ sv-num_rows = i;
*view = (MSIVIEW *)sv;
Re: dlls/shell32/pidl.c -- adjust to unsignedness of a type
On Sat, 16 Feb 2008, Alexandre Julliard wrote:
The test should probably be removed completely since there's already a
default for it in the switch() that follows.
I was thinking of this, but wanted to go for the smaller patch initially,
but your wish is my command ;-).
Updated patch below. Nearly everything here is whitespace changes; the
second hunk only removes if (type = 0 type = 2) { and }.
Gerald
ChangeLog:
Adjust a format specifier and remove a redundant range check in
ILGetDisplayNameExW().
Index: pidl.c
===
RCS file: /home/wine/wine/dlls/shell32/pidl.c,v
retrieving revision 1.159
diff -u -3 -p -r1.159 pidl.c
--- pidl.c 16 Feb 2008 15:58:53 - 1.159
+++ pidl.c 17 Feb 2008 18:44:54 -
@@ -97,7 +97,7 @@ BOOL WINAPI ILGetDisplayNameExW(LPSHELLF
STRRET strret;
DWORD flag;
-TRACE(%p %p %p %d\n, psf, pidl, path, type);
+TRACE(%p %p %p %x\n, psf, pidl, path, type);
if (!pidl || !path)
return FALSE;
@@ -109,46 +109,44 @@ BOOL WINAPI ILGetDisplayNameExW(LPSHELLF
return FALSE;
}
-if (type = 0 type = 2)
+switch (type)
{
-switch (type)
+case ILGDN_FORPARSING:
+flag = SHGDN_FORPARSING | SHGDN_FORADDRESSBAR;
+break;
+case ILGDN_NORMAL:
+flag = SHGDN_NORMAL;
+break;
+case ILGDN_INFOLDER:
+flag = SHGDN_INFOLDER;
+break;
+default:
+FIXME(Unknown type parameter = %x\n, type);
+flag = SHGDN_FORPARSING | SHGDN_FORADDRESSBAR;
+break;
+}
+
+if (!*(const WORD*)pidl || type == ILGDN_FORPARSING)
+{
+ret = IShellFolder_GetDisplayNameOf(lsf, pidl, flag, strret);
+if (SUCCEEDED(ret))
{
-case ILGDN_FORPARSING:
-flag = SHGDN_FORPARSING | SHGDN_FORADDRESSBAR;
-break;
-case ILGDN_NORMAL:
-flag = SHGDN_NORMAL;
-break;
-case ILGDN_INFOLDER:
-flag = SHGDN_INFOLDER;
-break;
-default:
-FIXME(Unknown type parameter = %x\n, type);
-flag = SHGDN_FORPARSING | SHGDN_FORADDRESSBAR;
-break;
+if(!StrRetToStrNW(path, MAX_PATH, strret, pidl))
+ret = E_FAIL;
}
-if (!*(const WORD*)pidl || type == ILGDN_FORPARSING)
+}
+else
+{
+ret = SHBindToParent(pidl, IID_IShellFolder, (LPVOID*)psfParent,
pidllast);
+if (SUCCEEDED(ret))
{
-ret = IShellFolder_GetDisplayNameOf(lsf, pidl, flag, strret);
+ret = IShellFolder_GetDisplayNameOf(psfParent, pidllast, flag,
strret);
if (SUCCEEDED(ret))
{
-if(!StrRetToStrNW(path, MAX_PATH, strret, pidl))
+if(!StrRetToStrNW(path, MAX_PATH, strret, pidllast))
ret = E_FAIL;
}
-}
-else
-{
-ret = SHBindToParent(pidl, IID_IShellFolder, (LPVOID*)psfParent,
pidllast);
-if (SUCCEEDED(ret))
-{
-ret = IShellFolder_GetDisplayNameOf(psfParent, pidllast, flag,
strret);
-if (SUCCEEDED(ret))
-{
-if(!StrRetToStrNW(path, MAX_PATH, strret, pidllast))
-ret = E_FAIL;
-}
-IShellFolder_Release(psfParent);
-}
+IShellFolder_Release(psfParent);
}
}
Re: dlls/gdi32/font.c warning removal (RESEND)
On Wed, 6 Feb 2008, Alexandre Julliard wrote: The reason is that we have #define HDPTOLP(y) ((y0)? \ (-abs(INTERNAL_YDSTOWS(dc, (y: \ (abs(INTERNAL_YDSTOWS(dc, (y) which is overly complicated for unsigned types. We can avoid these warnings by using plain INTERNAL_YDSTOWS instead of HDPTOLP here. No, you still need the abs(). You're right. That was a stupid thinko on my side. Please find below an updated patch. Gerald ChangeLog: For unsigned types, directly use INTERNAL_YDS Index: font.c === RCS file: /home/wine/wine/dlls/gdi32/font.c,v retrieving revision 1.39 diff -u -3 -p -r1.39 font.c --- font.c 6 Feb 2008 13:28:51 - 1.39 +++ font.c 10 Feb 2008 21:26:59 - @@ -1690,16 +1690,16 @@ UINT WINAPI GetOutlineTextMetricsW( output-otmTextMetrics.tmOverhang = WDPTOLP(output-otmTextMetrics.tmOverhang); output-otmAscent = HDPTOLP(output-otmAscent); output-otmDescent = HDPTOLP(output-otmDescent); - output-otmLineGap = HDPTOLP(output-otmLineGap); - output-otmsCapEmHeight = HDPTOLP(output-otmsCapEmHeight); - output-otmsXHeight = HDPTOLP(output-otmsXHeight); + output-otmLineGap = abs(INTERNAL_YDSTOWS(dc,output-otmLineGap)); + output-otmsCapEmHeight = abs(INTERNAL_YDSTOWS(dc,output-otmsCapEmHeight)); + output-otmsXHeight = abs(INTERNAL_YDSTOWS(dc,output-otmsXHeight)); output-otmrcFontBox.top = HDPTOLP(output-otmrcFontBox.top); output-otmrcFontBox.bottom = HDPTOLP(output-otmrcFontBox.bottom); output-otmrcFontBox.left = WDPTOLP(output-otmrcFontBox.left); output-otmrcFontBox.right = WDPTOLP(output-otmrcFontBox.right); output-otmMacAscent = HDPTOLP(output-otmMacAscent); output-otmMacDescent = HDPTOLP(output-otmMacDescent); - output-otmMacLineGap = HDPTOLP(output-otmMacLineGap); + output-otmMacLineGap = abs(INTERNAL_YDSTOWS(dc,output-otmMacLineGap)); output-otmptSubscriptSize.x = WDPTOLP(output-otmptSubscriptSize.x); output-otmptSubscriptSize.y = HDPTOLP(output-otmptSubscriptSize.y); output-otmptSubscriptOffset.x = WDPTOLP(output-otmptSubscriptOffset.x); @@ -1708,7 +1708,7 @@ UINT WINAPI GetOutlineTextMetricsW( output-otmptSuperscriptSize.y = HDPTOLP(output-otmptSuperscriptSize.y); output-otmptSuperscriptOffset.x = WDPTOLP(output-otmptSuperscriptOffset.x); output-otmptSuperscriptOffset.y = HDPTOLP(output-otmptSuperscriptOffset.y); - output-otmsStrikeoutSize = HDPTOLP(output-otmsStrikeoutSize); + output-otmsStrikeoutSize = abs(INTERNAL_YDSTOWS(dc,output-otmsStrikeoutSize)); output-otmsStrikeoutPosition = HDPTOLP(output-otmsStrikeoutPosition); output-otmsUnderscoreSize = HDPTOLP(output-otmsUnderscoreSize); output-otmsUnderscorePosition = HDPTOLP(output-otmsUnderscorePosition);
Re: dlls/d3d9/tests/visual.c -- address five compiler warnings (RESEND)
On Mon, 14 Jan 2008, Alexandre Julliard wrote: @@ -4348,12 +4349,12 @@ static void vshader_version_varying_test vs_3_0 returned color 0x%08x, expected 0x00203366\n, color); color = getPixelColor(device, 160, 360); ok((color 0x00ff) = 0x003c (color 0x00ff) = 0x004e - (color 0xff00) = 0x (color 0xff00) = 0x + (color 0xff00) = 0x (color 0x00ff) = 0x0066 (color 0x00ff) = 0x00210068, vs_1_1 returned color 0x%08x, expected 0x00808080\n, color); color = getPixelColor(device, 480, 360); ok((color 0x00ff) = 0x003c (color 0x00ff) = 0x004e - (color 0xff00) = 0x (color 0xff00) = 0x + (color 0xff00) = 0x (color 0x00ff) = 0x0066 (color 0x00ff) = 0x00210068, vs_2_0 returned color 0x%08x, expected 0x\n, color); These tests don't make much sense, with or without your fix, plus the error messages don't match the tests. This needs more work. It seems this was added with 2007-11-07 with the following log entry: Stefan Dösinger [EMAIL PROTECTED] wined3d: Shader Model 3.0 varying tests. Stefan, any chance you could look into this at one point? THanks, Gerald
Re: dlls/user32/edit.c -- remove superflous check (RESEND)
On Wed, 26 Dec 2007, Alexandre Julliard wrote:
dwBufLen already indictes that this variable is of type DWORD and
thus always positive. I also looked at the implementation of
ImmGetCompositionStringW() and we do not seem to return a negative
value (such as -1).
Maybe the implementation doesn't, but the function returns a LONG, and
can (and probably should) return negative values on error.
Okay. In that case, to account for possible future enhancements of
ImmGetCompositionStringW(), we'll need a patch like the following. The
current error handling code in EDIT_GetCompositionStr() simply doesn't
work due to the unsignedness of dwBufLen.
Gerald
ChangeLog:
Fix error handling in EDIT_GetCompositionStr().
Index: dlls/user32/edit.c
===
RCS file: /home/wine/wine/dlls/user32/edit.c,v
retrieving revision 1.16
diff -u -3 -p -r1.16 edit.c
--- dlls/user32/edit.c 20 Nov 2007 16:56:17 - 1.16
+++ dlls/user32/edit.c 26 Dec 2007 21:29:33 -
@@ -5360,6 +5348,7 @@ static void EDIT_UpdateText(EDITSTATE *e
static void EDIT_GetCompositionStr(HWND hwnd, LPARAM CompFlag, EDITSTATE *es)
{
+LONG l;
DWORD dwBufLen;
LPWSTR lpCompStr = NULL;
HIMC hIMC;
@@ -5369,9 +5358,9 @@ static void EDIT_GetCompositionStr(HWND
if (!(hIMC = ImmGetContext(hwnd)))
return;
-dwBufLen = ImmGetCompositionStringW(hIMC, GCS_COMPSTR, NULL, 0);
+dwBufLen = l = ImmGetCompositionStringW(hIMC, GCS_COMPSTR, NULL, 0);
-if (dwBufLen 0)
+if (l 0)
{
ImmReleaseContext(hwnd, hIMC);
return;
Re: dlls/comctl32/listview.c warning elimination
On Sun, 4 Nov 2007, Gerald Pfeifer wrote: The only patch I can think of that avoid *both* warnings is the one below. Not perfect, but I fair compromise. What do you think? Or do you have any better idea that we might want to try? For the record, your fix that you committed yesterday certainly looks simpler. A few cycles longer, but that isn't a performance critical path. Gerald
Re: PATCH: fix out of range array access in dlls/kernel32/relay16.c (fwd)
On Thu, 29 Nov 2007, Alexandre Julliard wrote: I checked again and if we don't address this we'll get two new warnings issues in a default build with GCC 4.3. How does the patch below look? Not good, it adds noise to the code for no good reason. Why would gcc complain about that one? GCC 4.3 (today's snapshot) complains as follows when building Wine with default options: relay16.c: In function 'relay_call_from_16': relay16.c:323: warning: array subscript is above array bounds relay16.c:427: warning: array subscript is above array bounds Looking at the code GCC is right: for (j = 0; j sizeof(call-ret)/sizeof(call-ret[0]); j++) if (call-ret[j] == 0xca66 || call-ret[j] == 0xcb66) break; if (call-ret[j] == 0xcb66) /* cdecl */ Unless we break out of the loop, after the loop j will be the number of elements in the array, and thus call-rej[j] will be the first element _after_ the array, running into the next field of the structure. The straightforward fix to avoid this out-of-array access was my first patch at http://www.winehq.org/pipermail/wine-patches/2007-September/044612.html which you didn't like too much ;-), so I cooked up the second one http://www.winehq.org/pipermail/wine-patches/2007-November/047288.html Do you (or does anyone else) have a better idea how to address this? Gerald
Re: Remove four useless checks in dlls/gdi32/enhmetafile.c (RESEND)
On Mon, 19 Nov 2007, Alexandre Julliard wrote:
I had expected this comment for a different patch of mine. In
dlls/gdi32/enhmetafile.c we are just reading existing records,
so I'm not sure what you have in mind here?
The records usually come from an external file, so they have to be
validated (not that this is done correctly everywhere, but we should
move towards more validation, not less).
I've been looking into this, and I'm afraid I'll need some help to
proceed. If you look at the code and my original patch below, you
will see that I removed four conditions which were noops, that is,
the compiler should (and could) simply remove them.
This is what my patch did at the source level.
If we want to add some input checking, I assume you would like to
check that these values are not too large? (They cannot be negative,
so the only range checking we can do is on the upper end.) How should
this look like? Any specific upper bounds you have in mind?
Or did I simply fail to explain my original patch, that is, convey the
point that this actually will not change program behavior?
Gerald
Index: dlls/gdi32/enhmetafile.c
===
RCS file: /home/wine/wine/dlls/gdi32/enhmetafile.c,v
retrieving revision 1.6
diff -u -3 -p -r1.6 enhmetafile.c
--- dlls/gdi32/enhmetafile.c3 Aug 2007 13:06:43 - 1.6
+++ dlls/gdi32/enhmetafile.c28 Nov 2007 23:18:34 -
@@ -1670,9 +1670,7 @@ BOOL WINAPI PlayEnhMetaFileRecord(
LPVOID lpPackedStruct;
/* check that offsets and data are contained within the record */
-if ( !( (lpCreate-cbBmi=0) (lpCreate-cbBits=0)
-(lpCreate-offBmi=0) (lpCreate-offBits=0)
-((lpCreate-offBmi +lpCreate-cbBmi ) = mr-nSize)
+if ( !( ((lpCreate-offBmi +lpCreate-cbBmi ) = mr-nSize)
((lpCreate-offBits+lpCreate-cbBits) = mr-nSize) ) )
{
ERR(Invalid EMR_CREATEDIBPATTERNBRUSHPT record\n);
Re: Remove four useless checks in dlls/gdi32/enhmetafile.c (RESEND)
On Sun, 2 Dec 2007, Alexandre Julliard wrote:
I'm aware of that, but the purpose of having these warnings is to spot
bugs, and when you find a bug you have to fix it. Yes, the checks
currently don't work, so they should be made to work, not removed. As
the comment says, you have to check that offsets and sizes are contained
within the record.
Doesn't the following part of the checks work and ensure that already?
if ( !( ((lpCreate-offBmi +lpCreate-cbBmi ) = mr-nSize)
((lpCreate-offBits+lpCreate-cbBits) = mr-nSize) ) )
My patch only removes the half of the original check which is a noop
(since the current code misses the fact that these variables always
will be = 0 by virtue of their type).
Ahhh! A lightbulb goes on. Since this is input from the outside, and
thus completely out of our control, you are worried about overflows, that
is, the sum of the two values (offset and size) being within range, but
not the individual parts.
Is this what you've been after? :-)
Updated patch below. (Now I only wonder whether the = in the original
code shouldn't have been , and thus the in my code shouldn't be =,
but that's a separate issue.)
Gerald
Index: dlls/gdi32/enhmetafile.c
===
RCS file: /home/wine/wine/dlls/gdi32/enhmetafile.c,v
retrieving revision 1.6
diff -u -3 -p -r1.6 enhmetafile.c
--- dlls/gdi32/enhmetafile.c3 Aug 2007 13:06:43 - 1.6
+++ dlls/gdi32/enhmetafile.c2 Dec 2007 23:32:50 -
@@ -1669,11 +1669,15 @@ BOOL WINAPI PlayEnhMetaFileRecord(
const EMRCREATEDIBPATTERNBRUSHPT *lpCreate = (const
EMRCREATEDIBPATTERNBRUSHPT *)mr;
LPVOID lpPackedStruct;
-/* check that offsets and data are contained within the record */
-if ( !( (lpCreate-cbBmi=0) (lpCreate-cbBits=0)
-(lpCreate-offBmi=0) (lpCreate-offBits=0)
-((lpCreate-offBmi +lpCreate-cbBmi ) = mr-nSize)
-((lpCreate-offBits+lpCreate-cbBits) = mr-nSize) ) )
+/* Check that offsets and data are contained within the record.
+ * The first four checks are not redundant -- think overflow!
+ */
+if ( lpCreate-offBmi mr-nSize
+ || lpCreate-cbBmimr-nSize
+ || lpCreate-offBits mr-nSize
+ || lpCreate-cbBits mr-nSize
+ || lpCreate-offBmi +lpCreate-cbBmi mr-nSize
+ || lpCreate-offBits+lpCreate-cbBits mr-nSize )
{
ERR(Invalid EMR_CREATEDIBPATTERNBRUSHPT record\n);
break;
dlls/wintab32/context.c -- simpflication and HEADS UP!
This change of mine is obvious since InExt and OutExt are DWORD, thus
unsigned, and thus always greater or equal zero so my patch should be
fine in any case.
However, looking at the else-part of the if-statement, I have some
doubts this is working as designed, and in general the abs() invocations
are bogus.
Any expert who could help with a more extensive patch?
Gerald
ChangeLog:
Simplify condition in ScaleForContext().
Index: dlls/wintab32/context.c
===
RCS file: /home/wine/wine/dlls/wintab32/context.c,v
retrieving revision 1.22
diff -u -3 -p -r1.22 context.c
--- dlls/wintab32/context.c 21 Sep 2007 12:24:49 - 1.22
+++ dlls/wintab32/context.c 30 Nov 2007 23:53:47 -
@@ -176,7 +176,7 @@ int TABLET_PostTabletMessage(LPOPENCONTE
static inline DWORD ScaleForContext(DWORD In, DWORD InOrg, DWORD InExt, DWORD
OutOrg, DWORD OutExt)
{
-if (((InExt 0 )(OutExt 0)) || ((InExt0) (OutExt 0)))
+if ((InExt 0) (OutExt 0))
return ((In - InOrg) * abs(OutExt) / abs(InExt)) + OutOrg;
else
return ((abs(InExt) - (In - InOrg))*abs(OutExt) / abs(InExt)) + OutOrg;
Re: Use GCC's -Wtype-limits
Hi Alexandre, On Wed, 21 Nov 2007, Alexandre Julliard wrote: If applied, I will commit to address, directly and by engaging others, the occurrences of such warnings in Wine. I'm not opposed to applying it, but you have to fix the warnings first, as far as possible the default build should not trigger any warnings. I have been submitting patches to address this, and got down the number quite a bit in my internal tree, but many of the patches I sent earlier this month haven't been applied or responded to yet except for those I resent. Should I continue resending the rest as well? GCC 4.3 hasn't been released yet and won't be released for a few months, so regular users/Wine contributors should not see these warnings, that's why I figured it would be good to get them in, for some of the more advanced hackers here to get exposure as well. Gerald 5B
