[PATCH v3 1/2] jscript: implement Enumerator()
Jacek Caban
jacek at codeweavers.com
Thu May 9 08:27:55 CDT 2019
Hi Andreas,
It's mostly looking good, but some details still need a bit more work.
On 5/6/19 10:21 PM, Andreas Maier wrote:
> Signed-off-by: Andreas Maier <staubim at quantentunnel.de>
> ---
> dlls/jscript/enumerator.c | 227 +++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 216 insertions(+), 11 deletions(-)
>
> diff --git a/dlls/jscript/enumerator.c b/dlls/jscript/enumerator.c
> index aa7737ac52..a7cad81c8a 100644
> --- a/dlls/jscript/enumerator.c
> +++ b/dlls/jscript/enumerator.c
> @@ -26,6 +26,11 @@ WINE_DEFAULT_DEBUG_CHANNEL(jscript);
>
> typedef struct {
> jsdisp_t dispex;
> + BOOL atend;
> + /* IEnumVARIANT returned by _NewEnum */
> + IEnumVARIANT *enumvar;
> + /* IEnumVARIANT current item */
> + VARIANT* enumitm;
There is no need to allocate enumitm (BTW, maybe call it just 'item'?)
separately. You could just store the structure inside
EnumeratorInstance. I would also suggest storing it as jsval_t. If you
just set it to jsval_undefined() when you're at the end, it could
simplify things even more.
> } EnumeratorInstance;
>
> static const WCHAR atEndW[] = {'a','t','E','n','d',0};
> @@ -33,43 +38,156 @@ static const WCHAR itemW[] = {'i','t','e','m',0};
> static const WCHAR moveFirstW[] = {'m','o','v','e','F','i','r','s','t',0};
> static const WCHAR moveNextW[] = {'m','o','v','e','N','e','x','t',0};
>
> +static inline EnumeratorInstance *enumerator_from_jsdisp(jsdisp_t *jsdisp)
> +{
> + return CONTAINING_RECORD(jsdisp, EnumeratorInstance, dispex);
> +}
> +
> +static inline EnumeratorInstance *enumerator_from_vdisp(vdisp_t *vdisp)
> +{
> + return enumerator_from_jsdisp(vdisp->u.jsdisp);
> +}
> +
> +static inline HRESULT enumvar_get_next_item(
> + IEnumVARIANT* enumvar, VARIANT* enumitm,
> + BOOL* atend, jsval_t *r)
I expect that passing EnumeratorInstance pointer instead of its fields
separately would make things easier.
> +{
> + HRESULT hres;
> +
> + /* not at end ... get next item */
> + if (!(*atend))
> + {
> + /* Assume valid variant */
> + VariantClear(enumitm);
> + hres = IEnumVARIANT_Next(enumvar, 1, enumitm, NULL);
> + if (hres != S_OK)
> + {
> + VariantClear(enumitm);
> + *atend = TRUE;
> + }
> + }
> +
> + if (*atend)
> + {
> + if (r)
> + *r = jsval_undefined();
> + return S_OK;
> + }
> +
> + if (r)
> + return variant_to_jsval(enumitm, r);
> +
> + return S_OK;
> +}
> +
> static void Enumerator_destructor(jsdisp_t *dispex)
> {
> + EnumeratorInstance *This;
> +
> TRACE("Enumerator_destructor\n");
>
> + This = enumerator_from_jsdisp(dispex);
> +
> + if (This->enumitm)
> + {
> + VariantClear(This->enumitm);
> + heap_free(This->enumitm);
> + }
> +
> heap_free(dispex);
> }
>
> HRESULT Enumerator_atEnd(script_ctx_t *ctx, vdisp_t *jsthis, WORD flags, unsigned argc, jsval_t *argv,
> jsval_t *r)
> {
> - TRACE("Enumerator_atEnd\n");
> + EnumeratorInstance *This;
> +
> + This = enumerator_from_vdisp(jsthis);
>
> - return E_NOTIMPL;
> + if (r)
> + *r = jsval_bool(This->atend);
> +
> + TRACE("Enumerator_atEnd %d\n", This->atend);
I missed it in already committed patch, but please don't add function
names to debug traces. Wine will add it for you.
> +
> + return S_OK;
> }
>
> HRESULT Enumerator_item(script_ctx_t *ctx, vdisp_t *jsthis, WORD flags, unsigned argc, jsval_t *argv,
> jsval_t *r)
> {
> + EnumeratorInstance *This;
> + HRESULT hres = E_FAIL;
> +
> TRACE("Enumerator_item\n");
>
> - return E_NOTIMPL;
> + This = enumerator_from_vdisp(jsthis);
You need to make sure that This is actually an enumerator instance.
JavaScript allows passing arbitrary values as this with something like
Enumerator.prototype.item.call(new Object());
See how it's handled in bool.c for an example (and don't worry about
exact error exceptions, FIXME(); return E_NOTIMPL; is fine for the first
iteration). Same applies to other functions.
> +
> + if (This->enumvar)
> + {
> + hres = variant_to_jsval(This->enumitm, r);
> + }
> + else
> + {
> + if (r)
> + *r = jsval_undefined();
> + hres = S_OK;
> + }
> +
> + return hres;
If you store the value as I suggested, it could be just:
return r ? jsval_copy(This->item, r) : S_OK;
> }
>
> HRESULT Enumerator_moveFirst(script_ctx_t *ctx, vdisp_t *jsthis, WORD flags, unsigned argc, jsval_t *argv,
> jsval_t *r)
> {
> + EnumeratorInstance *This;
> + HRESULT hres = E_FAIL;
> +
> TRACE("Enumerator_moveFirst\n");
>
> - return E_NOTIMPL;
> + This = enumerator_from_vdisp(jsthis);
> + if (This->enumvar)
> + {
> + IEnumVARIANT_Reset(This->enumvar);
We should probably check for Reset() error.
> + hres = enumvar_get_next_item(This->enumvar, This->enumitm, &This->atend, r);
Does it really return the item value here? It would need a test, but I'd
expect it to return undefined value.
> + }
> + else
> + {
> + if (r)
> + *r = jsval_undefined();
> + hres = S_OK;
> + }
> +
> + return hres;
> }
>
> HRESULT Enumerator_moveNext(script_ctx_t *ctx, vdisp_t *jsthis, WORD flags, unsigned argc, jsval_t *argv,
> jsval_t *r)
> {
> + EnumeratorInstance *This;
> + HRESULT hres = E_FAIL;
> +
> TRACE("Enumerator_moveNext\n");
>
> - return E_NOTIMPL;
> + This = enumerator_from_vdisp(jsthis);
> +
> + if (This->atend)
> + {
> + if (r)
> + *r = jsval_undefined();
> + return S_OK;
> + }
> + if (This->enumvar)
> + {
> + hres = enumvar_get_next_item(This->enumvar, This->enumitm, &This->atend, r);
Same here, should it return the value?
> @@ -180,11 +301,95 @@ HRESULT create_enumerator(script_ctx_t *ctx, jsval_t *argv, jsdisp_t **ret)
> {
> EnumeratorInstance *enumerator;
> HRESULT hres;
> + IDispatch *obj;
> + DISPPARAMS dispparams = {NULL, NULL, 0, 0};
> + VARIANT varresult;
> +
> + BOOL atend = TRUE;
> + IEnumVARIANT *enumvar = NULL;
> + VARIANT *enumitm = NULL;
> +
> + memset(&varresult, 0, sizeof(VARIANT));
> + VariantInit(&varresult);
> +
> + /* new Enumerator() */
> + if (argv == NULL)
> + {
> + atend = TRUE;
> + }
> + else if(is_object_instance(*argv))
> + {
> + obj = get_object(*argv);
> +
> + /* Try to get a IEnumVARIANT by _NewEnum */
> + hres = IDispatch_Invoke(obj,
> + DISPID_NEWENUM, &IID_NULL, LOCALE_NEUTRAL,
> + DISPATCH_METHOD, &dispparams, &varresult,
> + NULL, NULL);
> + if (FAILED(hres))
> + {
> + ERR("Enumerator: no DISPID_NEWENUM.\n");
> + hres = E_INVALIDARG;
> + goto cleanuperr;
> + }
> +
> + if (V_VT(&varresult) == VT_DISPATCH)
> + {
> + hres = IUnknown_QueryInterface(V_DISPATCH(&varresult),
> + &IID_IEnumVARIANT, (void**)&enumvar);
> + }
> + else if (V_VT(&varresult) == VT_UNKNOWN)
> + {
> + hres = IUnknown_QueryInterface(V_UNKNOWN(&varresult),
> + &IID_IEnumVARIANT, (void**)&enumvar);
> + }
Note that IDispatch inherits from IUnknown, so you could just handle
them as a single case and use V_UNKNOWN.
Thanks,
Jacek
More information about the wine-devel
mailing list