Re: [5/8] jscript: Implement the String.anchor() method.

[Jacek Caban]

Hi Andrew,

Andrew Nguyen wrote:
> dlls/jscript/string.c     |   58 ++++++++++++++++++++++++++++++++++++++++++
>  dlls/jscript/tests/api.js |   17 +++++++++++++
>  2 files changed, 73 insertions(+), 2 deletions(-)

+static HRESULT do_attribute_tag_format(DispatchEx *dispex, LCID lcid, WORD flags, DISPPARAMS *dp,
+        VARIANT *retv, jsexcept_t *ei, IServiceProvider *sp, const WCHAR *tagname, const WCHAR *attrname)
+{
+    static const WCHAR attrtagfmtW[] = {'<','%','s',' ','%','s','=','"','%','s','"','>','%','s','<','/','%','s','>',0};
+    static const WCHAR undefinedW[] = {'u','n','d','e','f','i','n','e','d',0};
+    StringInstance *string;
+    BSTR ret;
+    BSTR attrval = NULL;
+    DWORD len;
+    HRESULT hres = S_OK;
+
+    TRACE("\n");

IMO it would be better to change FIXME to TRACE in String_anchor instead of adding TRACE here. You use this function in a few places, and this TRACE doesn't tell us, which function is called.

+
+    if(!is_class(dispex, JSCLASS_STRING)) {
+        WARN("this is not a string object\n");
+        hres = E_NOTIMPL;
+        goto cleanup;

You may just return E_NOTIMPL here.

+    }
+
+    string = (StringInstance*)dispex;
+
+    if(retv) {
+        if(arg_cnt(dp)) {
+            hres = to_string(dispex->ctx, get_arg(dp, 0), ei, &attrval);

You should call to_string even if retv is NULL. Although it looks like a nice optimization, we can't do this. Eg, if to_string throws exception, we have to throw it here.

+            if (FAILED(hres))
+                goto cleanup;

You may just return hres here.

+        }
+
+        len = string->length + 2*strlenW(tagname) + strlenW(attrname) + 9;
+
+        if (attrval)
+            len += SysStringLen(attrval);
+        else
+            len += sizeof(undefinedW)/sizeof(WCHAR) - 1;
+
+        ret = SysAllocStringLen(NULL, len);
+
+        if(!ret)
+        {
+            hres = E_OUTOFMEMORY;
+            goto cleanup;

Perhaps it's just me, but in cases that don't make the code cleaner by using gotos, I'd avoid them. How about:

if(ret) {
    sprintfW(ret, attrtagfmtW, tagname, attrname, (attrval ? attrval : undefinedW), string->str, tagname);

V_VT(retv) = VT_BSTR;
    V_BSTR(retv) = ret;
}else {
    hres = E_OUTOFMEMORY;
}

+        }
+
+        sprintfW(ret, attrtagfmtW, tagname, attrname, (attrval ? attrval : undefinedW), string->str, tagname);
+
+        V_VT(retv) = VT_BSTR;
+        V_BSTR(retv) = ret;
+    }
+  cleanup:
+    SysFreeString(attrval);
+    return hres;
+}