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

[Christian Costa]

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

> On 11/07/2012 02:50 PM, Christian Costa wrote:
> >
> >
> > 2012/11/7 Nikolay Sivov <bunglehead at gmail.com <mailto:
> bunglehead at gmail.com>>
> >
> >     On 11/7/2012 01:05, Christian Costa wrote:
> >
> >         Le 06/11/2012 22:38, Michael Stefaniuc a écrit :
> >
> >             On 11/06/2012 08:51 PM, Christian Costa wrote:
> >
> >                 Le 06/11/2012 20:26, Nikolay Sivov a écrit :
> >
> >                     On 11/6/2012 20:47, Christian Costa wrote:
> >                     What I also meant is these names are redundant,
> >                     something like
> >                     directmusicloader_AddRef() is enough, of course you
> >                     could use mixed
> >                     casing in names if you want, there's no strict
> >                     guidelines for that afaik.
> >
> >                 I like to see the interface name in the function name
> >                 because the method
> >                 alone does not mean anything.
> >
> >                 Something like this:
> >                 <object name>Impl_<interface name>_<method name>
> >                 but in some case where there is only 1 interface for the
> >                 object:
> >                 <interface name>Impl_<method name>
> >                 is better.
> >
> >                 The interface name should be clearly visible IMO it's
> >                 easier to read in
> >                 a log file.
> >                 IDirectMusicLoaderImpl___IDirectMusicLoader_AddRef
> >
> >             I disagree. It is redundant, too long and thus hard to read.
> >             I even have patches that remove some of them. I've never
> >             submitted those
> >             as you beat me to the dm* COM cleanup.
> >
> >         I agree too if there is only one interface. I think it is but I
> >         would like to be sure before submitting a patch.
> >         I will check it and submit a patch for that.
> >         Too long and hard to read, I don't agree. See my response to
> >         Nikolay.
> >
> >     Your example of having two methods with the same name in same object
> >     could be different things:
> >
> >     - two separate not inherited interfaces will have IUnknown part in
> >     common at least, in this case it doesn't make sense
> >       to have traces from all of them, you use "main" interface to
> >     implement IUnknown part and forward to it.
> >
> >
> > I just use an example to show that we can have methods with the same
> > name. This other cases too off course.
> >
> >     Having methods like
> >       <maininterface>_<another interface>_AddRef is strange when you can
> >     reduce that to <another inerface>_AddRef
> >
> >
> > 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 ?


>
> >     - 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.
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
a functions table as it is done sometimes in strmbase but is less C++ like.


>
> 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.

Christian
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20121107/4e41049b/attachment-0001.html>