Re: [PATCH v4] vbscript: Implement Split.

[Jacek Caban]

Hi Robert,

The patch looks better now, thanks.

On 19.08.2020 22:34, Robert Wilhelm wrote:
> -
>    V_VT(&var) = VT_BSTR;
> -
>    V_BSTR(&var) = str;
> -
>    bounds.cElements++;
> -
>    SafeArrayRedim(sa, &bounds);
> -
>    hres = SafeArrayAccessData(sa, (void**)&data);
> -
>    if(FAILED(hres)) {
> -
>        SafeArrayDestroy(sa);
> -
>        break;
> -
>    }

Resizing safe array in each iteration is a bit suboptimal. Maybe we 
could have a temporary array of result strings (that we could grow 
exponentially) and construct the actual SAFEARRAY when we know the size.
> -
> -
>    hres = VariantCopyInd(data+count, &var);
> -
>    if(FAILED(hres)) {
> -
>        SafeArrayUnaccessData(sa);
> -
>        SafeArrayDestroy(sa);
> -
>        break;
> -
>    }
> -
> -
>    SafeArrayUnaccessData(sa);
> -
>    count++;
> -
> -
>    if (ret == -1) break;
> -
>    start = start + ret + delimeterlen;
> -
>    if (count == max) break;
> -
>    if (start > len) break;
> - }
> -
>
> +error:
> - if(res) {
> -
>    V_VT(res) = VT_ARRAY|VT_VARIANT;
> -
>    V_ARRAY(res) = sa;

We should set res value only when we return success. Otherwise the 
caller may assume that data is invalid and leak it.
> -
>
> }else {
> -
>    if (sa) SafeArrayDestroy(sa);
> -
>
> }
> -
> -
>
> if(V_VT(args) != VT_BSTR)
> -
>    SysFreeString(string);
> -
>
> if(V_VT(args+1) != VT_BSTR)
> -
>    SysFreeString(delimeter);
> -
>
> return hres;
> }
>
> static HRESULT Global_Replace(BuiltinDisp *This, VARIANT *args, unsigned args_cnt, VARIANT *res)
>
> diff --git a/dlls/vbscript/tests/api.vbs b/dlls/vbscript/tests/api.vbs
> index 6af49c849d..60ad6359dc 100644
> --- a/dlls/vbscript/tests/api.vbs
> +++ b/dlls/vbscript/tests/api.vbs
> @@ -609,6 +609,49 @@ TestLCase 0.123, doubleAsString(0.123)
>   TestLCase Empty, ""
>   Call ok(getVT(LCase(Null)) = "VT_NULL", "getVT(LCase(Null)) = " & getVT(LCase(Null)))
>
> +x=Split("abc")
> +Call ok(x(0) = "abc", "Split(""abc"")(0)=" & x(0))
> +x = Split("abc def")
> +Call ok(x(0) = "abc", "Split(""abc def"")(0)=" & x(0))
> +Call ok(x(1) = "def", "Split(""abc def"")(1)=" & x(1))
> +x = Split("abc def ghi")
> +Call ok(x(0) = "abc", "Split(""abc def ghi"")(0)=" & x(0))
> +Call ok(x(1) = "def", "Split(""abc def ghi"")(1)=" & x(1))
> +Call ok(x(2) = "ghi", "Split(""abc def ghi"")(2)=" & x(2))
> +x = Split("abc def","")
> +Call ok(x(0) = "abc def", "Split(""abc def"","""")(0)=" & x(0))
> +x = Split("abc-def","-")
> +Call ok(x(0) = "abc", "Split(""abc-def"",""-"")(0)=" & x(0))
> +Call ok(x(1) = "def", "Split(""abc-def"",""-"")(1)=" & x(1))
> +x = Split("abc--def","-")
> +Call ok(x(0) = "abc", "Split(""abc--def"",""-"")(0)=" & x(0))
> +Call ok(x(1) = "",    "Split(""abc--def"",""-"")(1)=" & x(1))
> +Call ok(x(2) = "def", "Split(""abc--def"",""-"")(2)=" & x(2))
> +x = Split("abcdefghi","def")
> +Call ok(x(0) = "abc", "Split(""abcdefghi"",""def"")(0)=" & x(0))
> +Call ok(x(1) = "ghi", "Split(""abcdefghi"",""def"")(1)=" & x(1))
> +x = Split("12345",3)
> +Call ok(x(0) = "12", "Split(""12345"",3)(0)=" & x(0))
> +Call ok(x(1) = "45", "Split(""12345"",3)(1)=" & x(1))
> +x = Split("12345",5)
> +Call ok(x(0) = "1234", "Split(""12345"",5)(0)=" & x(0))
> +Call ok(x(1) = "",     "Split(""12345"",5)(1)=" & x(1))
> +x = Split("12345",12)
> +Call ok(x(0) = "", "Split(""12345"",12)(0)=" & x(0))
> +Call ok(x(1) = "345", "Split(""12345"",12)(1)=" & x(1))
> +x = Split("abc-def-ghi","-")
> +Call ok(UBound(x) = 2, "UBound(Split(""abc-def-ghi"",""-""))=" & UBound(x))
> +x = Split("abc-def-ghi","-",2)
> +Call ok(UBound(x) = 1, "UBound(Split(""abc-def-ghi"",""-"",2))=" & UBound(x))
> +x = Split("abc-def-ghi","-",4)
> +Call ok(UBound(x) = 2, "UBound(Split(""abc-def-ghi"",""-"",4))="  & UBound(x))
> +x = Split("abcZdefZghi","Z",3,0)
> +Call ok(UBound(x) = 2, "UBound(Split(""abcZdefZghi"",""Z"",3,0))="  & UBound(x))
> +x = Split("abcZdefZghi","z",3,0)
> +Call ok(UBound(x) = 0, "UBound(Split(""abcZdefZghi"",""z"",3,0))=" & UBound(x))
> +x = Split("abcZdefZghi","z",3,1)
> +Call ok(UBound(x) = 2, "UBound(Split(""abcZdefZghi"",""z"",3,1))=" & UBound(x))

A test for negative "max" argument would be nice: both an explicit -1 
and some other value.

Thanks,

Jacek