Le 26/06/2015 08:57, Henner Zeller a écrit : > On 25 June 2015 at 23:45, jp charras <[email protected]> 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 : [email protected] >> Unsubscribe : https://launchpad.net/~kicad-developers >> More help : https://help.launchpad.net/ListHelp > -- 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
