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 _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
