Re: [PATCH] inetcomm/smtptransport.c : Remove some unneeded variables and assigns.
Hi Amine, 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. --Juan
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.
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. --Juan
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.
Well, that's entirely up to you guys ;)
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. --Juan
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.
On Wed, Dec 16, 2009 at 11:09 AM, Juan Lang <juan.lang(a)gmail.com> wrote:
Hi Amine,
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. --Juan
Also, you should never pass a function to a macro. What if FAILED were defined as: FAILED(x) (x & MASK1) | (x & MASK2) ? -- James Hawkins
2009/12/16 James Hawkins <truiken(a)gmail.com>:
Also, you should never pass a function to a macro. What if FAILED were defined as:
FAILED(x) (x & MASK1) | (x & MASK2)
?
Then the macro is broken, although admittedly most macros probably are.
participants (4)
-
Amine Khaldi -
Henri Verbeet -
James Hawkins -
Juan Lang