Re: [3/3] msxml3/domdoc: Add support for VT_ARRAY|VT_UI1 in domdoc_load(). (try 2)
On 5/5/2011 00:38, Adam Martinson wrote:
Fixes bugs 14864 + 16453. --- dlls/msxml3/domdoc.c | 27 +++++++++++++ dlls/msxml3/tests/domdoc.c | 94 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 120 insertions(+), 1 deletions(-) + case VT_ARRAY|VT_UI1: + { + SAFEARRAY *psa = V_ARRAY(&source); + xmlChar *str; + LONG len; + UINT dim = SafeArrayGetDim(psa); + + switch (dim) + { + case 0: + ERR("SAFEARRAY == NULL\n"); + hr = E_INVALIDARG; + break; + case 1: + /* Only takes UTF8 strings. + * NOT NULL-terminated. */ + SafeArrayAccessData(psa, (void**)&str); + SafeArrayGetUBound(psa, 1,&len); + hr = load_utf8(This, str, ++len, isSuccessful); + SafeArrayUnaccessData(psa); + break; + default: + FIXME("unhandled SAFEARRAY dim: %d\n", dim); + hr = E_NOTIMPL; + } + }
This could be simplified. SafeArrayGetUBound will fail for case 0 for example, and unhandled dimension should be WARN, IMO.
On 05/04/2011 06:04 PM, Nikolay Sivov wrote:
On 5/5/2011 00:38, Adam Martinson wrote:
Fixes bugs 14864 + 16453. --- dlls/msxml3/domdoc.c | 27 +++++++++++++ dlls/msxml3/tests/domdoc.c | 94 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 120 insertions(+), 1 deletions(-) + case VT_ARRAY|VT_UI1: + { + SAFEARRAY *psa = V_ARRAY(&source); + xmlChar *str; + LONG len; + UINT dim = SafeArrayGetDim(psa); + + switch (dim) + { + case 0: + ERR("SAFEARRAY == NULL\n"); + hr = E_INVALIDARG; + break; + case 1: + /* Only takes UTF8 strings. + * NOT NULL-terminated. */ + SafeArrayAccessData(psa, (void**)&str); + SafeArrayGetUBound(psa, 1,&len); + hr = load_utf8(This, str, ++len, isSuccessful); + SafeArrayUnaccessData(psa); + break; + default: + FIXME("unhandled SAFEARRAY dim: %d\n", dim); + hr = E_NOTIMPL; + } + }
This could be simplified. SafeArrayGetUBound will fail for case 0 for example, and unhandled dimension should be WARN, IMO. I think this is actually the most straightforward/readable way to handle it. If multi-dimensional SAFEARRAYs are actually supported, we will need separate cases for them. And IMHO if an app is trying to do something we don't handle, and don't know how it should be handled, it should be a FIXME.
On 5/5/2011 18:14, Adam Martinson wrote:
On 05/04/2011 06:04 PM, Nikolay Sivov wrote:
On 5/5/2011 00:38, Adam Martinson wrote:
Fixes bugs 14864 + 16453. --- dlls/msxml3/domdoc.c | 27 +++++++++++++ dlls/msxml3/tests/domdoc.c | 94 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 120 insertions(+), 1 deletions(-) + case VT_ARRAY|VT_UI1: + { + SAFEARRAY *psa = V_ARRAY(&source); + xmlChar *str; + LONG len; + UINT dim = SafeArrayGetDim(psa); + + switch (dim) + { + case 0: + ERR("SAFEARRAY == NULL\n"); + hr = E_INVALIDARG; + break; + case 1: + /* Only takes UTF8 strings. + * NOT NULL-terminated. */ + SafeArrayAccessData(psa, (void**)&str); + SafeArrayGetUBound(psa, 1,&len); + hr = load_utf8(This, str, ++len, isSuccessful); + SafeArrayUnaccessData(psa); + break; + default: + FIXME("unhandled SAFEARRAY dim: %d\n", dim); + hr = E_NOTIMPL; + } + }
This could be simplified. SafeArrayGetUBound will fail for case 0 for example, and unhandled dimension should be WARN, IMO. I think this is actually the most straightforward/readable way to handle it. If multi-dimensional SAFEARRAYs are actually supported, we will need separate cases for them. It's just not the way I thought about it, that's all. Ok, let it be like that, but could you remove ERR() from it? I feel like it will be useless line that never works, on the other hand we should add dumping of array dimension in debugstr_variant() so it will be instantly visible in trace. And IMHO if an app is trying to do something we don't handle, and don't know how it should be handled, it should be a FIXME. Well ok, I don't really expect it to come often.
On 05/05/2011 09:37 AM, Nikolay Sivov wrote:
On 5/5/2011 18:14, Adam Martinson wrote:
I think this is actually the most straightforward/readable way to handle it. If multi-dimensional SAFEARRAYs are actually supported, we will need separate cases for them. It's just not the way I thought about it, that's all. Ok, let it be like that, but could you remove ERR() from it? I feel like it will be useless line that never works, on the other hand we should add dumping of array dimension in debugstr_variant() so it will be instantly visible in trace. It's code that *shouldn't* ever execute AFAIK, that's why I used ERR(). And IMHO if an app is trying to do something we don't handle, and don't know how it should be handled, it should be a FIXME. Well ok, I don't really expect it to come often.
I don't either, but it makes debugging a lot easier if it ever does, lol. I think it's just good practice for API functions to complain when they don't know how to handle their args.
participants (2)
-
Adam Martinson -
Nikolay Sivov