On 04/26/14 16:38, Zhenbo Li wrote:
Hi Jacek,
2014-04-26 21:33 GMT+08:00 Jacek Caban <jacek(a)codeweavers.com>:
On 04/26/14 15:20, Zhenbo Li wrote:
2014-04-26 20:44 GMT+08:00 Jacek Caban <jacek(a)codeweavers.com>:
Hi Zhenbo,
This is better, but:
On 04/26/14 05:12, Zhenbo Li wrote:
+ hres = var2str(&v, &val); + + if (hres != S_OK){ + ERR("Set Width(%s) failed when initializing a nsAString!\n", + debugstr_variant(&v)); + nsAString_Finish(&val);
Again, this is not initialized in the error case. As I've added + default: + nsAString_Init(nsstr, NULL); + FIXME("unsupported arg %s\n", debugstr_variant(p)); + return E_NOTIMPL; + } Isn't it enough? I missed that change, but I think it's wrong. There are other error cases as well. nsAString_Init failure also leaves the string uninitialized (and it's on my TODO list to make sure that we may make nsAString_Init infailable, but that's a different story). It's also generally sane to expect that if a function fails, it's output parameter is not initialized. Please remove this new nsAString_Init call and nsAString_Finish in error case.
Thank you for your explanation. How about this solution:
+ return nsAString_Init(nsstr, V_BSTR(p))? + S_OK : E_OUTOFMEMORY; + case VT_R8: + hres = VarBstrFromR8(V_R8(p), 0, 0, &str); + break; + case VT_R4: + hres = VarBstrFromR4(V_R4(p), 0, 0, &str); + break; + case VT_I4: + hres = VarBstrFromI4(V_I4(p), 0, 0, &str); + break; + default: + FIXME("unsupported arg %s\n", debugstr_variant(p)); + hres = E_NOTIMPL; + break; + } + if (FAILED(hres)) + return hres; + + ret = nsAString_Init(nsstr, str); + SysFreeString(str); + return ret ? S_OK : E_OUTOFMEMORY; +}
This looks good.
Or you mean here?
+ ret = nsAString_Init(nsstr, str); + SysFreeString(str); + return ret ? S_OK : E_OUTOFMEMORY; Should I free a string which failed when initializing due to out-of-memory? Which one? The way it's done currently is right.
+ + hres = nsstr_to_truncated_bstr(&val, &bstr); + if (FAILED(hres)) { + SysFreeString(bstr);
And bstr is not initialized in error case. The error case comes from + ret = end ? SysAllocStringLen(str, end-str) : SysAllocString(str); + if(!ret) + return E_OUTOFMEMORY; I have the same question. Please read the code again. bstr is not initialized in this case, so you can't free it. Sorry, I made a mistake. When SysAllocString failes, should we mark bstr as NULL? Like this:
static HRESULT nsstr_to_truncated_bstr(const nsAString *nsstr, BSTR *ret_ptr) { //.... ret = end ? SysAllocStringLen(str, end-str) : SysAllocString(str);
*ret_ptr = ret; return ret ? S_OK : E_OUTOFMEMORY; }
You don't need this...
static HRESULT WINAPI HTMLTable_get_width(IHTMLTable *iface, VARIANT *p) { //.... hres = nsstr_to_truncated_bstr(&val, &bstr); if (FAILED(hres)) return hres; //... }
...once you change this. Jacek