Re: [2/5] msxml3: Remove appendChild() forward (try2)

[Jacek Caban]

On 2/23/11 1:21 PM, Nikolay Sivov wrote:
> - TRACE("(%p)->(%p %p)\n", This, newChild, outNewChild);
> -
> - hr = IXMLDOMNode_get_nodeType(newChild,&type);
> - hr = IXMLDOMNode_get_nodeType(child,&type);
>  if(FAILED(hr) || type == NODE_ATTRIBUTE) {
> -
>    if(outNewChild) *outNewChild = NULL;
> -
>    if (outChild) *outChild = NULL;
>     return E_FAIL;
> }

This check for an attribute node shouldn't be needed here. Attribute 
node may just return failure instead of forwarding the call. Also I'd 
expect the same from other nodes that can't have children, like text node.

Jacek

[2/5] msxml3: Remove appendChild() forward (try2)

[Nikolay Sivov]

On 2/23/2011 15:25, Jacek Caban wrote:
> On 2/23/11 1:21 PM, Nikolay Sivov wrote:
> > - TRACE("(%p)->(%p %p)\n", This, newChild, outNewChild);
> > -
> > - hr = IXMLDOMNode_get_nodeType(newChild,&type);
> > - hr = IXMLDOMNode_get_nodeType(child,&type);
> >  if(FAILED(hr) || type == NODE_ATTRIBUTE) {
> > -
> >    if(outNewChild) *outNewChild = NULL;
> > -
> >    if (outChild) *outChild = NULL;
> >     return E_FAIL;
> > }
>
> This check for an attribute node shouldn't be needed here. Attribute 
> node may just return failure instead of forwarding the call. Also I'd 
> expect the same from other nodes that can't have children, like text 
> node.

It's a check for a child, not for a This node.
> Jacek

[2/5] msxml3: Remove appendChild() forward (try2)

[Jacek Caban]

On 2/23/11 1:27 PM, Nikolay Sivov wrote:
> On 2/23/2011 15:25, Jacek Caban wrote:
> > On 2/23/11 1:21 PM, Nikolay Sivov wrote:
> > > - TRACE("(%p)->(%p %p)\n", This, newChild, outNewChild);
> > > -
> > > - hr = IXMLDOMNode_get_nodeType(newChild,&type);
> > > - hr = IXMLDOMNode_get_nodeType(child,&type);
> > >  if(FAILED(hr) || type == NODE_ATTRIBUTE) {
> > > -
> > >    if(outNewChild) *outNewChild = NULL;
> > > -
> > >    if (outChild) *outChild = NULL;
> > >     return E_FAIL;
> > > }
> >
> > This check for an attribute node shouldn't be needed here. Attribute 
> > node may just return failure instead of forwarding the call. Also I'd 
> > expect the same from other nodes that can't have children, like text 
> > node.
>
> It's a check for a child, not for a This node.

Oh, right, sorry for too quick response.

Jacek