EPICS Controls Argonne National Laboratory

Experimental Physics and
Industrial Control System

2002  2003  2004  2005  2006  2007  2008  2009  <20102011  2012  2013  2014  2015  2016  2017  2018  2019  2020  2021  2022  2023  2024  Index 2002  2003  2004  2005  2006  2007  2008  2009  <20102011  2012  2013  2014  2015  2016  2017  2018  2019  2020  2021  2022  2023  2024 
<== Date ==> <== Thread ==>

Subject: Re: [Merge] lp:~anj/epics-base/expand-calc-size into lp:epics-base
From: "Ben F." <[email protected]>
To: Ralph Lange <[email protected]>
Date: Thu, 07 Oct 2010 17:07:40 -0000
On Donnerstag, 7. Oktober 2010, Ralph Lange wrote:
> Looks good to me.
>
> But I'd prefer someone with a more intimate knowledge of the calc to
> have another look. Ben, would you?!

I have taken a close look and it all looks correct. That said, I do have 
some reservations about the idea of using a split double/integer 
representation just to save some bytes of memory; there is a runtime 
cost (case distinction + conversion) and also the cost in code 
complexity to consider (the latter is small, though).

There are some things I would have done differently. For instance I do 
not think that

  while ((op = *pinst++) != END_EXPRESSION){
    ...using op instead of *pinst...
  }

is an improvement over

  while (*pinst != END_EXPRESSION){
    ...
    ++pinst;
  }

(Such a changes was made in two places in calcPerform.c). This might be 
a matter of personal style, though, so I do not have any strong 
objections.

I did *not* check whether the new INFIX_TO_POSTFIX_SIZE macro is correct 
(I did not even understand the old one). It would be nice to add a 
somewhat longer comment that thoroughly explains how this expression is 
derived from the postfix conversion algorithm. This would make it 
easier to adapt it in a correct way when further changes are made.

Cheers
Ben
-- 
"Never confuse what is natural with what is habitual."
                                                 -- Mahatma Gandhi
https://code.launchpad.net/~anj/epics-base/expand-calc-size/+merge/37507
Your team EPICS Core Developers is requested to review the proposed merge of lp:~anj/epics-base/expand-calc-size into lp:epics-base.


References:
Re: [Merge] lp:~anj/epics-base/expand-calc-size into lp:epics-base Ralph Lange

Navigate by Date:
Prev: Re: [Merge] lp:~anj/epics-base/expand-calc-size into lp:epics-base Ralph Lange
Next: 3.14.12 Feature Freeze Andrew Johnson
Index: 2002  2003  2004  2005  2006  2007  2008  2009  <20102011  2012  2013  2014  2015  2016  2017  2018  2019  2020  2021  2022  2023  2024 
Navigate by Thread:
Prev: Re: [Merge] lp:~anj/epics-base/expand-calc-size into lp:epics-base Ralph Lange
Next: Launchpad Bugs Re-enabling Auto Expiring Bugs deryck . hodge
Index: 2002  2003  2004  2005  2006  2007  2008  2009  <20102011  2012  2013  2014  2015  2016  2017  2018  2019  2020  2021  2022  2023  2024 
ANJ, 02 Feb 2012 Valid HTML 4.01! · Home · News · About · Base · Modules · Extensions · Distributions · Download ·
· Search · EPICS V4 · IRMIS · Talk · Bugs · Documents · Links · Licensing ·