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

[Christian Costa]

2012/11/7 Nikolay Sivov <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.



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


>
> For that particular case dmloader_ is enough to prefix names IMO.


Sorry I disagree but it seems it's more a matter of taste. Personally I
never get annoyed with line size
as I remember.



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


So apart from const problem it's more a style question. I'm ok with that.
If you have looked even more thoroughly on my patches,
you should have noticed that I started to clean this when it does not
increased patch size too much.

BTW which whitespaces ?


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.
>
>
> I already know that. None of my patches use then and I clean them when I
can like in my second patch you didn't read.
I repeat once again. I didn't write this code.

Just for the story I recently rename hwnd to wnd and lppnt to point when
modifying ClientToScreen and I was told it was not needed.
So that would be good if reviewers agree on the same rules and put them
somewhere in the wiki as well as cleanup task (as it was done for COM
cleanup).

And please look at the whole serie before commenting and review what the
patch does and not the badness of the previous code.

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