[PATCH 0/1] MR3132: vbscript: Coerce to VT_BOOL when evaluating jump conditions.
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3132
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- dlls/vbscript/interp.c | 41 +++++++++-------------- dlls/vbscript/tests/lang.vbs | 63 ++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 26 deletions(-) diff --git a/dlls/vbscript/interp.c b/dlls/vbscript/interp.c index eb1dcab5f01..7b77916f96c 100644 --- a/dlls/vbscript/interp.c +++ b/dlls/vbscript/interp.c @@ -413,43 +413,32 @@ static HRESULT stack_assume_val(exec_ctx_t *ctx, unsigned n) return S_OK; } -static int stack_pop_bool(exec_ctx_t *ctx, BOOL *b) +static HRESULT stack_pop_bool(exec_ctx_t *ctx, BOOL *b) { variant_val_t val; HRESULT hres; - VARIANT_BOOL vb; + VARIANT v; hres = stack_pop_val(ctx, &val); if(FAILED(hres)) return hres; - switch (V_VT(val.v)) + if (V_VT(val.v) == VT_NULL) { - case VT_BOOL: - *b = V_BOOL(val.v); - break; - case VT_NULL: - case VT_EMPTY: *b = FALSE; - break; - case VT_I2: - *b = V_I2(val.v); - break; - case VT_I4: - *b = V_I4(val.v); - break; - case VT_BSTR: - hres = VarBoolFromStr(V_BSTR(val.v), ctx->script->lcid, 0, &vb); - if(FAILED(hres)) - return hres; - *b=vb; - break; - default: - FIXME("unsupported for %s\n", debugstr_variant(val.v)); - release_val(&val); - return E_NOTIMPL; } - return S_OK; + else + { + V_VT(&v) = VT_EMPTY; + hres = VariantChangeType(&v, val.v, VARIANT_LOCALBOOL, VT_BOOL); + } + + release_val(&val); + + if (SUCCEEDED(hres)) + *b = V_BOOL(&v) == VARIANT_TRUE; + + return hres; } static HRESULT stack_pop_disp(exec_ctx_t *ctx, IDispatch **ret) diff --git a/dlls/vbscript/tests/lang.vbs b/dlls/vbscript/tests/lang.vbs index 77e132133b0..af662533265 100644 --- a/dlls/vbscript/tests/lang.vbs +++ b/dlls/vbscript/tests/lang.vbs @@ -420,6 +420,69 @@ else end if Call ok(x = 1, "if ""-1"" not executed") +x = 0 +if 0.1 then + x = 1 +else + ok false, "if ""0.1"" else executed" +end if +Call ok(x = 1, "if ""0.1"" not executed") + +x = 0 +if "TRUE" then + x = 1 +else + ok false, "if ""TRUE"" else executed" +end if +Call ok(x = 1, "if ""TRUE"" not executed") + +x = 0 +if "#TRUE#" then + x = 1 +else + ok false, "if ""#TRUE#"" else executed" +end if +Call ok(x = 1, "if ""#TRUE#"" not executed") + +x = 0 +if (not "#FALSE#") then + x = 1 +else + ok false, "if ""not #FALSE#"" else executed" +end if +Call ok(x = 1, "if ""not #FALSE#"" not executed") + +Class ValClass + Public myval + + Public default Property Get defprop + defprop = myval + End Property +End Class + +Dim MyObject +Set MyObject = New ValClass + +MyObject.myval = 1 +Call ok(CBool(MyObject) = True, "CBool(MyObject) = " & CBool(MyObject)) +x = 0 +if MyObject then + x = 1 +else + ok false, "if ""MyObject(1)"" else executed" +end if +Call ok(x = 1, "if ""MyObject(1)"" not executed") + +MyObject.myval = 0 +Call ok(CBool(MyObject) = False, "CBool(MyObject) = " & CBool(MyObject)) +x = 0 +if not MyObject then + x = 1 +else + ok false, "if ""MyObject(0)"" else executed" +end if +Call ok(x = 1, "if ""MyObject(0)"" not executed") + x = 0 WHILE x < 3 : x = x + 1 : Wend Call ok(x = 3, "x not equal to 3") -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3132
@nsivov, @jacek Hello. Just following up as I was migrating my code to the latest version of wine. Is there a reason this one was never merged? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3132#note_43550
On Mon Aug 28 13:08:18 2023 +0000, Jason Millard wrote:
@nsivov, @jacek Hello. Just following up as I was migrating my code to the latest version of wine. Is there a reason this one was never merged? There are CI failures.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3132#note_43552
On Mon Aug 28 13:08:18 2023 +0000, Jacek Caban wrote:
There are CI failures. Yes, there are definitely some failures on the bot, but I don't see them locally. Maybe somehow particular toolchain affects this, if you @jsm174 could take a look, that would be great.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3132#note_43553
On Mon Aug 28 13:12:26 2023 +0000, Nikolay Sivov wrote:
Yes, there are definitely some failures on the bot, but I don't see them locally. Maybe somehow particular toolchain affects this, if you @jsm174 could take a look, that would be great. Okay, I will look. Can you rebase this?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3132#note_43555
On Mon Aug 28 13:28:13 2023 +0000, Jason Millard wrote:
Okay, I will look. Can you rebase this? It applies cleanly as far as I can tell.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3132#note_43568
On Mon Aug 28 13:39:50 2023 +0000, Nikolay Sivov wrote:
It applies cleanly as far as I can tell. Yes, I thought you could just rebase this MR with the rebase button above, as it wouldn't let me do it. I'll just clone latest, and retry.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3132#note_43569
On Mon Aug 28 13:41:11 2023 +0000, Jason Millard wrote:
Yes, I thought you could just rebase this MR with the rebase button above, as it wouldn't let me do it. I'll just clone latest, and retry. I had a MR started before this one, so I just rebased and cherry picked yours. (Couldn't figure out to get CI to run on my fork).
Anyway, it looks like the linux32 errored in a completely different place, but the vbscript ones worked fine: https://gitlab.winehq.org/jsm174/wine/-/jobs/26720#L2279 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3132#note_43589
On Mon Aug 28 19:56:30 2023 +0000, Jason Millard wrote:
I had a MR started before this one, so I just rebased and cherry picked yours. (Couldn't figure out to get CI to run on my fork). Anyway, it looks like the linux32 errored in a completely different place, but the vbscript ones worked fine: https://gitlab.winehq.org/jsm174/wine/-/jobs/26720#L2279 Yes, CI looks good now, perhaps the problem was somewhere else and it's fixed now, there is not much we can do being unable to reproduce it. The patch looks mostly fine, with one minor comment.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3132#note_43693
Jacek Caban (@jacek) commented about dlls/vbscript/interp.c:
- default: - FIXME("unsupported for %s\n", debugstr_variant(val.v)); - release_val(&val); - return E_NOTIMPL; } - return S_OK; + else + { + V_VT(&v) = VT_EMPTY; + hres = VariantChangeType(&v, val.v, VARIANT_LOCALBOOL, VT_BOOL); + } + + release_val(&val); + + if (SUCCEEDED(hres)) + *b = V_BOOL(&v) == VARIANT_TRUE; Please avoid comparison to `VARIANT_TRUE`, that makes values like `TRUE` behave like `VARIANT_FALSE`. Comparing to `VARIANT_FALSE` would be better, or perhaps something `!!V_BOOL(&v)`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3132#note_43694
participants (4)
-
Jacek Caban (＠jacek) -
Jason Millard (＠jsm174)
-
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)