<br><br><div class="gmail_quote">2012/11/7 Nikolay Sivov <span dir="ltr"><<a href="mailto:bunglehead@gmail.com" target="_blank">bunglehead@gmail.com</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

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

- 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_<u></u>GetFontCollection() and dwritetextlayout_<u></u>GetFontCollection(). But that is more an exception because of dwrite C++ nature;<br>
</blockquote><div><br></div><div>The method name is GetFontCollection but what is the interface ? ILayout, ITextLayout, ... I have to dig in the code and do some grep.</div><div>And why don't you use getfontcollection instead, or gfntcollec?</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
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.<br></blockquote><div><br></div><div>It seems "clean" is somewhat subjective. I prefer also short line in log but this meaningfull name.</div>
<div>Just take quartz with object that can have up to 10 interfaces and interfaces that can be implemented by up to 10 objects.</div><div>Put a bit of multithreading on top of it (and I don't mention the fact the TRACE are not serialized between thread).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
For that particular case dmloader_ is enough to prefix names IMO.</blockquote><div><br></div><div>Sorry I disagree but it seems it's more a matter of taste. Personally I never get annoyed with line size</div><div>as I remember.</div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
If dmloader object has really 1 interface, I would rename then<br>
IDirectMusicLoaderImpl_AddRef.<br>
Although this is probably the case, I'm not sure yet. In all cases, I<br>
would leave this for another patch. One thing at once.<br>
</blockquote>
Sure, separate patch to remove the duplicate name is fine. Just start<br>
with that as it makes the subsequent COM cleanup patch shorter as it<br>
cleans up the function header (whitespace and LPJUNK).<br>
</blockquote>
That's what I try to do altough I don't clearly understand what the problem with LPJUNK stuff.<br>
</blockquote></div>
Why would you use LPVOID, LPDWORD or LPCWSTR when it could be cleanly expressed without typedefs?</blockquote><div> </div><div>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,</div>
<div>you should have noticed that I started to clean this when it does not increased patch size too much. </div><div><br></div><div>BTW which whitespaces ? </div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>


<br><br></blockquote><div>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.</div><div>I repeat once again. I didn't write this code.</div><div><br>
</div><div>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.</div><div>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).</div>
<div><br></div><div>And please look at the whole serie before commenting and review what the patch does and not the badness of the previous code.</div></div><br>
<div>Christian</div>