Re: Shortcut Rational addition if either operand is zero (issue 551690046 by address@hidden)

[hanwenn]

https://codereview.appspot.com/551690046/diff/573690043/flower/rational.cc
File flower/rational.cc (right):

https://codereview.appspot.com/551690046/diff/573690043/flower/rational.cc#newcode257
flower/rational.cc:257: if (is_infinity ())
On 2020/04/12 16:55:05, Dan Eble wrote:
> The hazard of reviewing this class is that there's always something to
wonder
> about.  This time, I wonder about adding +infinity and -infinity
together (in
> either order).

yes. I recommend against doing that :-)

https://codereview.appspot.com/551690046/diff/573690043/flower/rational.cc#newcode261
flower/rational.cc:261: else if (r.sign_ == 0)
On 2020/04/12 16:55:05, Dan Eble wrote:
> Is this the only micro-optimization you measured?  Other ideas you
might wish to
> try:
> 
> * move the conditions that read this->sign_ together
> 
> * are infinities handled first because they are significantly more
common than
> zeros?  try moving the branches that test for zero before the branches
that test
> for infinities
> 
> * inline a subset of the shortcuts in rational.hh
> 
> In any case, noting your findings in a concise comment would help
future
> maintainers avoid erasing your gains.

It's the only one I investigated.  The rational addition shows up
because we add Moments together in many places. Since those generally
have grace == 0, it's doing a lot of unwarranted
multiplication/division.

I'll move the zero check first, because it's more likely than the
infinity check.

https://codereview.appspot.com/551690046/