Re: aclui: Remove some unneeded header inclusions. (IWYU)

[Amine Khaldi]

On 06/09/2013 16:28, Alexandre Julliard wrote:
 Amine Khaldi amine.kha...@reactos.org writes:

 @@ -18,15 +18,7 @@
   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
   */
  
 -#include config.h
 -
 -#include stdarg.h
 -
  #include initguid.h
 -#include windef.h
 -#include winbase.h
 -#include winuser.h
 -#include winnt.h
  #include aclui.h
 That's not useful. The headers will be included anyway.

Indeed. Are there some benefits of including them more than once here ?

Regards,
Amine.

Re: [PATCH] activeds: Exclude unused headers.

[Amine Khaldi]

On 12/03/2013 11:27, Alexandre Julliard wrote:
 Amine Khaldi amine.kha...@reactos.org writes:

 @@ -28,13 +28,10 @@
  
  #include windef.h
  #include winbase.h
 -#include winuser.h
  
  #include objbase.h
  #include iads.h
 -#include adshlp.h
  
 -#include wine/unicode.h
  #include wine/debug.h
 It's not worth the trouble, particularly since the file ends up
 including windows.h anyway.

In this case ending up with windows.h is not needed, so if we:

#define WIN32_NO_STATUS
#define _INC_WINDOWS
#define COM_NO_WINDOWS_H

That prevents it.

Should I send a second patch with these defines ?

Regards,
Amine.

Coverity Scans Revived

[Amine Khaldi]

Hello folks,

Recently we (myself and Urias from Haiku) have been working on getting 
the coverity scans up and running again. I'm happy to announce that we 
succeeded ! :)


I submitted a scan and the results are up online. I'll submit further 
scans when necessary, like it's already done with clang analyzer scans.


Have fun !

Regards,
Amine.

Static analysis scan

[Amine Khaldi]

Hello folks,

I've set up a static analysis scan using clang static analyzer. The 
results are available at 
http://austinenglish.com/logs/clang_analyzer/index.html thanks to Austin 
for the web space.


Please feel free to fix the defects. I'll run future scans when the 
defects get fixed progressively.


Have fun ;)

With best regards,
Amine.

Re: hlink/hlink_main.c : Remove an unneeded assignment.

[Amine Khaldi]

Hi Amine, 

Hi Jacek,




  r = register_clsid(CLSID_StdHlink);
  if (SUCCEEDED(r))
 -r = register_clsid(CLSID_StdHlinkBrowseContext);
 +register_clsid(CLSID_StdHlinkBrowseContext);

  return S_OK;



In this case the correct fix is to return r. Please be more careful 
then doing janitorial work. There are more places in your patches that 
seem questionable. You should make sure that we want to ignore an 
error instead of handling it (and usually we don't) before deleting 
such assignments. Otherwise it's just hiding bugs.


Hmm.. I missed this point, sorry.

Thanks,
Amine.

Re: inetcomm/smtptransport.c : Remove some unneeded variables and assigns.

[Amine Khaldi]

Out of curiosity, what tool are you using to find these bugs?

Clang.

WBR,
Amine.

Re: inetcomm/smtptransport.c : Remove some unneeded variables and assigns.

[Amine Khaldi]

Hey Chip,


I think he's using Clang. I've seen him on the LLVM bugzilla. He's
waiting for someone (e.g. me) to fix the bugs that prevent Wine from
being compiled with Clang.

That's right.


BTW, if and when you find a bug using Clang, be sure to put
(Clang/LLVM) in the title--or at least mention that you used Clang.

Noted.

WBR,
Amine.

Re: [PATCH] inetcomm/smtptransport.c : Remove some unneeded variables and assigns.

[Amine Khaldi]

Hi Amine,
   

Hi Juan,


this patch has no functional benefit.  For example,
-HRESULT hr;

  TRACE(\n);

-hr = SMTPTransport_ParseResponse(This, pBuffer,response);
-if (FAILED(hr))
+if (FAILED(SMTPTransport_ParseResponse(This, pBuffer,response)))

This has the dubious benefit of removing a local variable that was
used before.  I can't fathom why this is a good thing.
   


It makes the code use one less variable. I don't see how is this 
dubious, but I understand that it's trivial.


WBR,
Amine.

Re: [PATCH] inetcomm/smtptransport.c : Remove some unneeded variables and assigns.

[Amine Khaldi]

It makes the code use one less variable. I don't see how is this dubious,
but I understand that it's trivial.
 

Yes, at the cost of potentially less readability, or, depending on the
compile settings, a little more difficulty in checking whether the
function succeeded.  I don't see that the benefits outweigh the costs.
   

Well, that's entirely up to you guys ;)

Amine.

Re: [PATCH] inetcomm/smtptransport.c : Fix a typo ?

[Amine Khaldi]

Hi Amine,
   

Hi Juan,


Um, it's called a prototype, and it's needed to compile
smtptransport.c.  Have you tested this patch at all?
   
I knew it was a prototype, I just thought it was left over, as it wasn't 
at the top of the file, and I saw no other prototype in that file. As 
for testing, I was eventually going to re-compile, so I guess I'd figure 
this out later if you didn't mention it.


Amine.

Re: [PATCH] inetcomm/smtptransport.c : Remove some unneeded variables and assigns.

[Amine Khaldi]

Not really, it's up to you too.  This is essentially a style-only
change.  Alexandre generally frowns on these, but reluctantly accepts
them if the existing style is horrible, or if you're actively involved
in the code being modified.  Neither appears to be true here.  This is
a helpful hint to avoid you killing your AJ-rank unnecessarily:  don't
do that, your patches are less likely to get accepted in the future.
   
Thank you for the hint, I wasn't aware of this (I'm sort of a new 
comer).. noted.

AJ-rank, hein ? :)

Amine.