Re: [PATCH] tests: Don't evaluate the todo condition when not running on Wine.
On 21.04.2017 15:01, Huw Davies wrote:
Signed-off-by: Huw Davies <huw(a)codeweavers.com> --- This cropped up because I wanted to write a todo condition involving IsProcessDPIAware(), since this doesn't exist in XP, its address is loaded dynamically. I could have written the condition as pIsProcessDPIAware && pIsProcessDPIAware(), but since I know the function is always present in Wine, that's unnecessary. Perhaps more to the point, I didn't expect the current behaviour.
include/wine/test.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/wine/test.h b/include/wine/test.h index af602c0fac..fa3f90c932 100644 --- a/include/wine/test.h +++ b/include/wine/test.h @@ -123,7 +123,7 @@ extern void __winetest_cdecl winetest_trace( const char *msg, ... ) WINETEST_PRI winetest_loop_todo(); \ winetest_end_todo()) #define todo_wine todo_if(!strcmp(winetest_platform, "wine")) -#define todo_wine_if(is_todo) todo_if((is_todo) && !strcmp(winetest_platform, "wine")) +#define todo_wine_if(is_todo) todo_if(!strcmp(winetest_platform, "wine") && (is_todo))
#ifdef NONAMELESSUNION
I don't mind if its changed, but please note that it was intentional to check "is_todo" first. The idea was that side effects (like assigning variables, changing last error, ...) are applied on both Wine and Windows. Just think about something like: FirstCall(); todo_wine_if(SecondCall()) ok(GetLastError() == ..., "\n"); With your patch applied, it will test two entirely different things on Windows (error from FirstCall) and Wine (error from SecondCall), which would also be confusing. Best regards, Sebastian
On Fri, Apr 21, 2017 at 03:31:14PM +0200, Sebastian Lackner wrote:
On 21.04.2017 15:01, Huw Davies wrote:
Signed-off-by: Huw Davies <huw(a)codeweavers.com> --- This cropped up because I wanted to write a todo condition involving IsProcessDPIAware(), since this doesn't exist in XP, its address is loaded dynamically. I could have written the condition as pIsProcessDPIAware && pIsProcessDPIAware(), but since I know the function is always present in Wine, that's unnecessary. Perhaps more to the point, I didn't expect the current behaviour.
include/wine/test.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/wine/test.h b/include/wine/test.h index af602c0fac..fa3f90c932 100644 --- a/include/wine/test.h +++ b/include/wine/test.h @@ -123,7 +123,7 @@ extern void __winetest_cdecl winetest_trace( const char *msg, ... ) WINETEST_PRI winetest_loop_todo(); \ winetest_end_todo()) #define todo_wine todo_if(!strcmp(winetest_platform, "wine")) -#define todo_wine_if(is_todo) todo_if((is_todo) && !strcmp(winetest_platform, "wine")) +#define todo_wine_if(is_todo) todo_if(!strcmp(winetest_platform, "wine") && (is_todo))
#ifdef NONAMELESSUNION
I don't mind if its changed, but please note that it was intentional to check "is_todo" first. The idea was that side effects (like assigning variables, changing last error, ...) are applied on both Wine and Windows. Just think about something like:
FirstCall(); todo_wine_if(SecondCall()) ok(GetLastError() == ..., "\n");
With your patch applied, it will test two entirely different things on Windows (error from FirstCall) and Wine (error from SecondCall), which would also be confusing.
Yeah, I'm aware that this will make a difference in a case like this, but we should avoid relying on side effects of the condition. I'm not particularly bothered either way by this patch, so I'm not going to argue for it too much ;-) I suppose the problem here is that it looks too much like a straight 'if', which is really isn't. Huw.
Huw Davies <huw(a)codeweavers.com> writes:
On Fri, Apr 21, 2017 at 03:31:14PM +0200, Sebastian Lackner wrote:
On 21.04.2017 15:01, Huw Davies wrote:
Signed-off-by: Huw Davies <huw(a)codeweavers.com> --- This cropped up because I wanted to write a todo condition involving IsProcessDPIAware(), since this doesn't exist in XP, its address is loaded dynamically. I could have written the condition as pIsProcessDPIAware && pIsProcessDPIAware(), but since I know the function is always present in Wine, that's unnecessary. Perhaps more to the point, I didn't expect the current behaviour.
include/wine/test.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/wine/test.h b/include/wine/test.h index af602c0fac..fa3f90c932 100644 --- a/include/wine/test.h +++ b/include/wine/test.h @@ -123,7 +123,7 @@ extern void __winetest_cdecl winetest_trace( const char *msg, ... ) WINETEST_PRI winetest_loop_todo(); \ winetest_end_todo()) #define todo_wine todo_if(!strcmp(winetest_platform, "wine")) -#define todo_wine_if(is_todo) todo_if((is_todo) && !strcmp(winetest_platform, "wine")) +#define todo_wine_if(is_todo) todo_if(!strcmp(winetest_platform, "wine") && (is_todo))
#ifdef NONAMELESSUNION
I don't mind if its changed, but please note that it was intentional to check "is_todo" first. The idea was that side effects (like assigning variables, changing last error, ...) are applied on both Wine and Windows. Just think about something like:
FirstCall(); todo_wine_if(SecondCall()) ok(GetLastError() == ..., "\n");
With your patch applied, it will test two entirely different things on Windows (error from FirstCall) and Wine (error from SecondCall), which would also be confusing.
Yeah, I'm aware that this will make a difference in a case like this, but we should avoid relying on side effects of the condition. I'm not particularly bothered either way by this patch, so I'm not going to argue for it too much ;-)
Note that the platform being set to "wine" doesn't guarantee that you are running on Wine, or that you are running against the builtin dll. It's not likely to be an issue in this case, but you should still verify the pointer before calling it. -- Alexandre Julliard julliard(a)winehq.org
On Fri, Apr 21, 2017 at 03:49:42PM +0200, Alexandre Julliard wrote:
Huw Davies <huw(a)codeweavers.com> writes:
On Fri, Apr 21, 2017 at 03:31:14PM +0200, Sebastian Lackner wrote:
I don't mind if its changed, but please note that it was intentional to check "is_todo" first. The idea was that side effects (like assigning variables, changing last error, ...) are applied on both Wine and Windows. Just think about something like:
FirstCall(); todo_wine_if(SecondCall()) ok(GetLastError() == ..., "\n");
With your patch applied, it will test two entirely different things on Windows (error from FirstCall) and Wine (error from SecondCall), which would also be confusing.
Yeah, I'm aware that this will make a difference in a case like this, but we should avoid relying on side effects of the condition. I'm not particularly bothered either way by this patch, so I'm not going to argue for it too much ;-)
Note that the platform being set to "wine" doesn't guarantee that you are running on Wine, or that you are running against the builtin dll. It's not likely to be an issue in this case, but you should still verify the pointer before calling it.
Ah yes. Ok, let's drop this. Thanks, Huw.
participants (3)
-
Alexandre Julliard -
Huw Davies -
Sebastian Lackner