On Thu, Jun 07, 2018 at 09:15:02PM +0100, Owen Rudge wrote:
On 07/06/2018 08:56, Huw Davies wrote:
+static BOOL move_to_element(WS_XML_READER *reader, const char
*element_name, WS_XML_STRING *uri)
return HRESULT
+static BOOL ws_element_to_wsdxml_element(WS_XML_READER *reader,
IWSDXMLContext *context, WSDXML_ELEMENT *parent_element)
and here.
int read_message(const char *xml, int xml_length, WSD_SOAP_MESSAGE
**out_msg)
This is also looking like a good contender to return HRESULT and take an [out] int *type parameter.
As per the previous patch, the HRESULT is not ultimately used, and I can't see that there's a great benefit in passing it along. I do understand it for the other functions that ultimately return a value to the caller, but these functions are all called by the listener thread, and a simple identification of the message type is all we require.
If you think it would be beneficial, I can modify these functions to return HRESULT, but it would be good to understand the benefits of doing so.
It's mainly just for consistency. I don't feel particularly strongly about it in these cases, however in previous patch-sets I let similar things go, only for it later to become apparent that I shouldn't have.
One way to look at it is that conceptually the function has no idea that the caller cannot use the failure status, so it should return it regardless.
- ok(any != NULL, "%s: any == NULL\n", debug_prefix); + todo_wine ok(any != NULL, "%s: any == NULL\n", debug_prefix);
You'll need to have a really good reason for doing this. Can you re-order things so you don't break the tests?
This will be fixed in the next patchset; I didn't want to include any more code in what's already quite a large patch. There's probably an extra 40 or so lines of code needed to avoid this breakage. There's not really a way I can re-order this that I can think of.
If preferred though, I can submit the patch that will fix this as part of this patchset, so the net result once the set is committed involves no extra todos.
It certainly can't be left like this at the end of the patch-set. It might be ok (since this isn't exactly a core dll) to include the next patch so that the implementation doesn't regress as far as the tests are concerned.
Huw.