kicad-developers team mailing list archive

[jp charras <jp.charras@xxxxxxxxxx>]

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 )
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