Re: [PATCH 1/3] msxml3: Return xmlnode object from create_basic_node.

[Alexandre Julliard]

Jacek Caban <jacek(a)codeweavers.com> writes:
> @@ -84,6 +83,13 @@ static inline xmlnode *impl_from_IXMLDOMNode( IXMLDOMNode *iface )
>      return (xmlnode *)((char*)iface - FIELD_OFFSET(xmlnode, lpVtbl));
>  }
>
> +static inline IXMLDOMNode *_IXMLDOMNode_(xmlnode *This)
> +{
> +    return (IXMLDOMNode*)&This->lpVtbl;
> +}

That wrapper doesn't follow any established naming convention, and
doesn't seem quite necessary. The standard way is to simply cast to the
interface type. If you really want a wrapper it should be called
something like IXMLDOMNode_from_impl.

-- 
Alexandre Julliard
julliard(a)winehq.org

[PATCH 1/3] msxml3: Return xmlnode object from create_basic_node.

[Jacek Caban]

Alexandre Julliard wrote:
> Jacek Caban <jacek(a)codeweavers.com> writes:
> > @@ -84,6 +83,13 @@ static inline xmlnode *impl_from_IXMLDOMNode( IXMLDOMNode *iface )
> >      return (xmlnode *)((char*)iface - FIELD_OFFSET(xmlnode, lpVtbl));
> >  }
> >
> > +static inline IXMLDOMNode *_IXMLDOMNode_(xmlnode *This)
> > +{
> > +    return (IXMLDOMNode*)&This->lpVtbl;
> > +}
>
> That wrapper doesn't follow any established naming convention,

I'm sure I saw it somewhere in Wine and I liked it. AFAICS there are no 
object to interface cast wrappers in msxml3 (except for dispex.c, which 
is copied from mshtml), so I thought I could introduce it here.
> and doesn't seem quite necessary. The standard way is to simply cast to the
> interface type.

I just feel that it's better to have a wrapper to avoid explicit casts 
in the code and have vtbl logic separated from other code. I usually use 
macros, but I know you don't like it, so I used an inline function here.
> If you really want a wrapper it should be called
> something like IXMLDOMNode_from_impl.

I will resend these patches without the wrapper.


Thanks,
    Jacek

[PATCH 1/3] msxml3: Return xmlnode object from create_basic_node.

[Alexandre Julliard]

Jacek Caban <jacek(a)codeweavers.com> writes:
> I just feel that it's better to have a wrapper to avoid explicit casts
> in the code and have vtbl logic separated from other code. I usually
> use macros, but I know you don't like it, so I used an inline function
> here.

For secondary vtbls that's definitely a good idea (as long as you are
using inline functions and not macros...)  The main vtbl is a bit
different because by design its address and the object are the same
thing, so you can use a cast and ignore the fact that there even is a
vtbl involved. Though I'm not opposed to a function wrapper either, just
with a more explicit name.

-- 
Alexandre Julliard
julliard(a)winehq.org

[PATCH 1/3] msxml3: Return xmlnode object from create_basic_node.

[Jacek Caban]

Alexandre Julliard wrote:
> Jacek Caban <jacek(a)codeweavers.com> writes:
> > I just feel that it's better to have a wrapper to avoid explicit casts
> > in the code and have vtbl logic separated from other code. I usually
> > use macros, but I know you don't like it, so I used an inline function
> > here.
>
> For secondary vtbls that's definitely a good idea (as long as you are
> using inline functions and not macros...)  The main vtbl is a bit
> different because by design its address and the object are the same
> thing, so you can use a cast and ignore the fact that there even is a
> vtbl involved.

That's one of things in the design that I'd like to change. This 
assumption doesn't work well for object inheritance.
> Though I'm not opposed to a function wrapper either, just
> with a more explicit name.

I will use IXMLDOMNode_from_impl then.


Thanks,
    Jacek