Re: [PATCH v2] vbscript: Implement Split.

[Jacek Caban]

Hi Robert,

Thanks for the patch, I have a few comments bellow.

On 16.08.2020 23:55, Robert Wilhelm wrote:
> Wine-Bug:https://bugs.winehq.org/show_bug.cgi?id=47570
> Signed-off-by: Robert Wilhelm
>
> dlls/vbscript/global.c      | 119 +++++++++++++++++++++++++++++++++++-
>   dlls/vbscript/tests/api.vbs |  19 ++++++
>   2 files changed, 135 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/vbscript/global.c b/dlls/vbscript/global.c
> index 122f5c7b71..b799b962fe 100644
> --- a/dlls/vbscript/global.c
> +++ b/dlls/vbscript/global.c
> @@ -2291,10 +2291,123 @@ static HRESULT Global_Join(BuiltinDisp *This, VARIANT *arg, unsigned args_cnt, V
>       return E_NOTIMPL;
>   }
>
> -static HRESULT Global_Split(BuiltinDisp *This, VARIANT *arg, unsigned args_cnt, VARIANT *res)
> +static HRESULT Global_Split(BuiltinDisp *This, VARIANT *args, unsigned args_cnt, VARIANT *res)
>   {
> - FIXME("\n");
> - return E_NOTIMPL;
> - BSTR str,string,delimeter;
> - int count,max,mode,len,start,end,ret;
> - SAFEARRAYBOUND bounds;
> - SAFEARRAY *sa;
> - VARIANT *data,var;
> - HRESULT hres = S_OK;
> -
> - TRACE("%s %u...\n", debugstr_variant(args), args_cnt);
> -
> - assert(1 <= args_cnt && args_cnt <= 4);
> -
> - if(V_VT(args) != VT_BSTR) {
> -
>    hres = to_string(args, &string);

string is never freed.
> -
>    if(FAILED(hres))
> -
>        return hres;
> - }else {
> -
>    string = V_BSTR(args);
> - }
> -
> - if ( args_cnt > 1)
> - {
> -
>    if(V_VT(args+1) != VT_BSTR) {
> -
>        hres = to_string(args+1, &delimeter);

delimiter is never freed.
> -
>        if(FAILED(hres))
> -
>            return hres;
> -
>    }else {
> -
>        delimeter = V_BSTR(args+1);
> -
>    }
> - }
> - else
> - delimeter = SysAllocString(L" ");

In this case, we don't really need to allocate it. I'd suggest to change 
how delimiter is handled to not require BSTR in this case.
> -
> - if(args_cnt > 2) {
> -
>    hres = to_int(args + 2, &max);
> -
>    if(FAILED(hres))
> -
>        return hres;
> - }else {
> -
>    max = -1;
> - }
> -
> - if (args_cnt == 4) {
> -
>    hres = to_int(args+3, &mode);
> -
>    if(FAILED(hres))
> -
>        return hres;
> -
> -
>    if (mode != 0 && mode != 1) {
> -
>        FIXME("unknown compare mode = %d\n", mode);
> -
>        return E_FAIL;
> -
>    }
> - }
> - else
> -
>    mode = 0;
> -
> - start = 0;
> -
> - bounds.lLbound = 0;
> - bounds.cElements = 0;
> -
> - sa = SafeArrayCreate( VT_VARIANT, 1, &bounds);
> - if (!sa)
> -
>    return E_OUTOFMEMORY;

This is leaked in error code paths.
> -
> - len = SysStringLen(string);
> - count = 0;
> -
> - while(1)
> - {
> -
>    ret = FindStringOrdinal(FIND_FROMSTART, string + start, len -start,
> -
>                            delimeter, SysStringLen(delimeter), mode);
> -
> -
>    if (ret == -1)
> -
>        end = len;
> -
>    else
> -
>        end = start + ret;
> -
> -
>    str = SysAllocStringLen(string + start, end - start);
> -
>    if (!str)
> -
>        return E_OUTOFMEMORY;
> -
> -
>    V_VT(&var) = VT_BSTR;
> -
>    V_BSTR(&var) = str;
> -
> -
>    bounds.cElements++;
> -
>    SafeArrayRedim(sa, &bounds);
> -
>    hres = SafeArrayAccessData(sa, (void**)&data);
> -
>    if(FAILED(hres)) {
> -
>        SafeArrayDestroy(sa);
> -
>        return hres;
> -
>    }
> -
> -
>    hres = VariantCopyInd(data+count, &var);
> -
> -
>    if(FAILED(hres)) {
> -
>        SafeArrayUnaccessData(sa);
> -
>        SafeArrayDestroy(sa);
> -
>        return hres;
> -
>    }
> -
> -
>    SafeArrayUnaccessData(sa);
> -
> -
>    if (ret == -1) break;
> -
> -
>    start = start + ret + SysStringLen(delimeter);
> -
>    count++;
> -
>    if (count == max) break;
> - }
> -
> - if(res) {
> -
>    V_VT(res) = VT_ARRAY|VT_VARIANT;
> -
>    V_ARRAY(res) = sa;
> - }else {
> -
>    SafeArrayDestroy(sa);
> - }
> -
> - return S_OK;
> }

Also please try to use consistent white space style. The patch uses a 
few inconsistently, ideally it would use similar to existing code in the 
file.
> 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..fe4799b52f 100644
> --- a/dlls/vbscript/tests/api.vbs
> +++ b/dlls/vbscript/tests/api.vbs
> @@ -609,6 +609,25 @@ 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 returned " & x(0))
> +x = Split("abc def")
> +Call ok(x(0) = "abc", "Split returned " & x(0))
> +Call ok(x(1) = "def", "Split returned " & x(1))
> +x = Split("abc-def","-")
> +Call ok(x(0) = "abc", "Split returned " & x(0))
> +Call ok(x(1) = "def", "Split returned " & x(1))
> +x = Split("abc-def-ghi","-")
> +Call ok(UBound(x) = 2, "Split returned " & UBound(x))
> +x = Split("abc-def-ghi","-",2)
> +Call ok(UBound(x) = 1, "Split returned " & UBound(x))
> +x = Split("abcZdefZghi","Z",3,0)
> +Call ok(UBound(x) = 2, "Split returned " & UBound(x))
> +x = Split("abcZdefZghi","z",3,0)
> +Call ok(UBound(x) = 0, "Split returned " & UBound(x))
> +x = Split("abcZdefZghi","z",3,1)
> +Call ok(UBound(x) = 2, "Split returned " & UBound(x))
> -

It would be interesting to see a bit more tests. For example, empty ("") 
delimiter, delimiter with more than one character, a case that returns 
more than 2 elements.

Thanks,

Jacek