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