kicad-developers team mailing list archive

[Henner Zeller <h.zeller@xxxxxxx>]

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.

> 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