[PATCH 1/3] dmloader: COM cleanup of IDirectMusicLoader object.

Christian Costa titan.costa at gmail.com
Thu Nov 8 05:48:12 CST 2012


2012/11/8 Michael Stefaniuc <mstefani at redhat.com>

> On 11/07/2012 06:40 PM, Christian Costa wrote:
> > 2012/11/7 Michael Stefaniuc <mstefani at redhat.com
> > <mailto:mstefani at redhat.com>>
> >     On 11/07/2012 02:50 PM, Christian Costa wrote:
> >     > I didn't write this code and I don't like the current name either
> >     but a
> >     > method name does not mean anything if you don't know the interface
> it
> >     > belongs to.
> >     > And I would put the real interface name as for the method. It's a
> bit
> >     > like renaming GetBufferDataPointer method to getbuf just because
> it's
> >     > shorter.
> >     Actually that's what Nikolay said. The function name needs to be
> >     <interface>Impl_<method> where interface/method are the official
> names
> >     from the API. What is superfluous/redundant information is to prepend
> >     all that with the object name. Which in the case of dm* is the name
> of
> >     the primary interface + "Impl".
> >
> >
> > So in this particular case: IDirectMusicLoaderImpl_AddRef, right ?
> Right.
>
> >     >     - object implements interface that overrides some of methods
> >     from an
> >     >     interface it's inherited, this is a bit special and in dwrite
> for
> >     >     example it's done like
> >     dwritetextlayout_layout___GetFontCollection()
> >     >     and dwritetextlayout___GetFontCollection(). But that is more an
> >     >     exception because of dwrite C++ nature;
> >     >
> >     >
> >     > The method name is GetFontCollection but what is the interface ?
> >     > ILayout, ITextLayout, ... I have to dig in the code and do some
> grep.
> >     > And why don't you use getfontcollection instead, or gfntcollec?
> >     >
> >     >
> >     >     Why it's good to have short and clean names? Because that will
> get
> >     >     you clean traces where name doesn't eat half of line width.
> >     >
> >     >
> >     > It seems "clean" is somewhat subjective. I prefer also short line
> >     in log
> >     > but this meaningfull name.
> >     > Just take quartz with object that can have up to 10 interfaces and
> >     > interfaces that can be implemented by up to 10 objects.
> >     > Put a bit of multithreading on top of it (and I don't mention the
> fact
> >     > the TRACE are not serialized between thread).
> >     I the case of quartz most of the duplicated interfaces are
> implemented
> >     by the same base (C++) object. Or should be implemented that way. So
> >     while multiple COM classes support the same interface most of the
> time
> >     there is one implementation only.
> >
> >
> > It's partly true. Sometimes some methods are overridden.
> >
> >
> >
> >
> >     If not the next question to ask is if it really makes sense to have
> two
> >     monster COM classes in the same .c file.
> >
> >
> > Yes of course but you will have the same function name in the debugger
> and
> > in the trace.
> I'll pass on the debugger as I'm not a debugger person.
>
> TRACE on the other hand is simple. Sometimes there is a different debug
> channel in use. Or the trace message itself can contain the info. But I
> also made the experience that more often than not one has to follow the
> interface/object pointer from the TRACE to be able to distinguish
> between different instances of the same COM class. I mean follow it back
> to the creation/initialization of the object which gives the needed info
> to find the right implementation.
>

Relying on that just because we use functions with the same name is awkward
imo.

>
> > For example in dmusic/port.c there is 3 port objects for Synth,
> > MidiOut and MidiIn each one supported 3 interfaces
> > IDirectMusicPort, IDirectMusicPassThru
> > and possibly IDirectMusicPortDownload. Altough it is ok to use for
> > example IDirectMusicPortImpl_<Method>
> > for the common methods implementation but you will need to specify the
> > object name
> > for the methods that are specific to each port. Unless you keep a
> > generic implementation and use
> I've seen different techniques in use, it really depends on how big the
> implementations differ. It might be even solvable with an if-else
>     if (This->has_ds8) // if is_primary(This)
>         foo;
>     else
>         bar;
>
> Other times there is a different vtable with different degrees of
> methods in common.
>

I know these techniques and all are good. The best one to use depends
on the situation.


>
> > a functions table as it is done sometimes in strmbase but is less C++
> like.
> You make it sound like "less C++ like" would be bad. Quite the contrary,
> not being bound by the C++ object model gives you flexibility. There was
> a great LWN article on the object oriented programming/design patterns
> in the Linux Kernel, it is worth a read for anybody interested in object
> oriented programming in C.
>

Well no. Was just a comment. I'm not a pro C++.
That said naming conventions should not be a constraint to choose a
particular
technique over another one.


>
> >     Of course Alexandre might not be convinced about a file split but in
> >     that case to avoid functions with the same name use something short
> to
> >     prefix it. I prefer using the upper letters of the COM class as a
> >     prefix, e.g.: dsb_IUnknown_QueryInterface as opposed to
> >     IDirectSoundBuffer8Impl_IUnknown_QueryInterface. By the time I get to
> >     the method name I have forgotten the starting part of the function
> name.
> >     Also I don't get confused by seeing two interface names in the same
> >     function name.
> >
> > Personally I would have used :
> > dsb_IUnknown_QueryInterface
> > dsb_IDirectSoundBuffer8_QueryInterface
> > dsb_Ixxxx_yyyy
> >
> > So we have object_interface_method. This is more formal and clear and we
> > can remove the "Impl" suffix.
> > Only one simple rule and if we keep the object name short the function
> > name is not too long.
> We did think about using the object name, but there is no COM object
> naming standard in Wine and some of the object names could be better.
> Using object_interface_method could yield stuff like
> "struct_IFooBarImpl_IFooBar_QueryInterface()" which isn't a beauty.
>

Well why struct_I*Impl ?
It seems there is a confusion between class and interface.
For example CLSID_DirectMusicLoader is the class and IDirectMusicLoader is
the interface.
Off course there are tightly tied together because CLSID_DirectMusicLoader
has only 1 interface and
IDirectMusicLoader is only implement by this class.

In that case I would just use _IDirectMusicLoader_QueryInterface() (with an
underscore prefix)
That would work also for an generic interface whose implementation is
shared by several classes.

And if we have to make a class distinction we just add the class name so we
have dmload_IDirectMusicLoader_QueryInterface.

So we have the pattern (class)_interface_method() which work in every
situation and enable homogeneity.

This concept of primary interface is a bit odd imo. Saying that IBaseFilter
is the primary interface of the video renderer does not
make sense.


>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20121108/1458cdd0/attachment-0001.html>


More information about the wine-devel mailing list