[PATCH 4/4] wsdapi: Add initial support for reading messages; identify Probe message.

[Huw Davies]

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.