kicad-developers team mailing list archive

[jp charras <jp.charras@xxxxxxxxxx>]

Le 26/06/2015 08:57, Henner Zeller a écrit :
> On 25 June 2015 at 23:45, jp charras <jp.charras@xxxxxxxxxx> wrote:
>> Le 26/06/2015 07:40, Chris Pavlina a écrit :
>>> Hi,
>>>
>>> macros.h provides two macros that appear to me completely unnecessary:
>>> EXCHG and NEGATE.
>>>
>>> The comment above EXCHG points out that it differs from std::swap in
>>> that it accepts arguments of different types. I could not find a single
>>> instance depending on that - it always swapped values of the same type.
>>> It also remarks "I hope to get rid of this soon or late".  I removed it
>>> and replaced its uses with std::swap.
>>>
>>> The comment above NEGATE justifies it:
>>>
>>> // This really needs a function? well, it is used *a lot* of times
>>>
>>> Doesn't really mean it needs a macro though, otherwise we might as well
>>> go defining macros like DOUBLE() and INCREMENT() and so on... It's just
>>> as easy to type something like "x = -x" or "x *= -1", and the latter is
>>> _shorter_ than "NEGATE( x )". I removed this too.
>>>
>>> Up to you whether or not you want to accept the patch, but I think it's
>>> a step in the direction of cleaner code. Macros for simple things that
>>> don't need them confuse people - you have to go check the definition to
>>> see if it does something "special". They also result in some clumsy use
>>> cases like this, from trying to shove them in:
>>>
>>> m_pos.x -= aYaxis_position;
>>> NEGATE(  m_pos.x );
>>> m_pos.x += aYaxis_position;
>>>
>>> Writing out the NEGATE makes it clear that the lines are all very
>>> similar and can easily be simplified:
>>>
>>> m_pos.x = 2 * aYaxis_position - m_pos.x;
>>>
>>> That's a lot clearer... I don't know about you, but I can visualize what
>>> that's doing graphically a lot better too.
>>>
>>> --
>>> Chris
>>
>> I fully agree to replace EXCHG by std::swap ( EXCHG come from the early
>> times when Kicad was written in C).
>>
>>
>> I am *not* convinced by replacing
>>> m_pos.x -= aYaxis_position;
>>> NEGATE(  m_pos.x );
>>> m_pos.x += aYaxis_position;
>>
>> by
>>> m_pos.x = 2 * aYaxis_position - m_pos.x;
>>
>> When you read the last form, it is not clear this is a mirroring
>> relative to the aYaxis_position X position.
>>
>> At least for me, it is not "a lot clearer...", this is quite the opposite.
>>
>> Perhaps a macro like MIRROR( m_pos.x, aYaxis_position )
> 
> .. or a type-safe inline function.

Yes, you are right!


> 
>> is "a lot clearer..."
>>
>> By the way, for me:
>> "x = -x" looks better than "x *= -1"
>> an should be faster to calculate.
>>
>> Thanks.
>>
>> --
>> Jean-Pierre CHARRAS
>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~kicad-developers
>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~kicad-developers
>> More help   : https://help.launchpad.net/ListHelp
> 


-- 
Jean-Pierre CHARRAS