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

[Nikolay Sivov]

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. Having methods like
   <maininterface>_<another interface>_AddRef is strange when you can 
reduce that to <another inerface>_AddRef
- 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;

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.

For that particular case dmloader_ is enough to prefix names IMO.
>
>>
>>> If dmloader object has really 1 interface, I would rename then
>>> IDirectMusicLoaderImpl_AddRef.
>>> Although this is probably the case, I'm not sure yet. In all cases, I
>>> would leave this for another patch. One thing at once.
>> Sure, separate patch to remove the duplicate name is fine. Just start
>> with that as it makes the subsequent COM cleanup patch shorter as it
>> cleans up the function header (whitespace and LPJUNK).
> That's what I try to do altough I don't clearly understand what the 
> problem with LPJUNK stuff.
Why would you use LPVOID, LPDWORD or LPCWSTR when it could be cleanly 
expressed without typedefs? Also naming convention of having 
type-dependent prefix like dwLen or lpcszFileName is just a noise, why 
have 5 letters prefix that supposed to express variable type, I'll check 
type anyway, and lpcszFileName turns to filename with no info loss.