Experimental Physics and Industrial Control System
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
<2010>
2011
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
<2010>
2011
2012
2013
2014
2015
2016
2017
2018
2019
2020
2021
2022
2023
2024