iraf.net

In CDL, there's a few sequence point issues in eps.c in the functions eps_writeMono() and eps_writeMonoRGB(). I think I know what was intended by the code but since the actual behavior of the code is not well-defined, I have to ask what the intended behavior is. Both sections use the MONO macro from eps.h:



The parameters of this macro are missing their parenthesis guards and it should itself be changed to



to prevent unintuitive behavior. For example using the current definition, if x and y are some values, MONO(x+y,0,0) would not in general be equal to MONO((x+y),0,0). (Also, off the top of my head I don't know where the ".33R+ .5G+ .17B" values come from and if they are calculated or ad hoc.)

In eps_writeMono(), here's the offending statement:



I think this line is intended to do



Similarly, in eps_writeMonoRGB(), there are two statements like this



I think this line, which is far more problematic than the previous one, is intended as



Currently, when compiled, these statements issue sequence-point warnings from gcc:


Since by definition the interpretation of these statements is compiler dependent, I cannot say for sure what the intended meaning. I think the intended behavior is obvious and that I've guessed correctly but could I get confirmation that:

a) the MONO macro's change is good
b) my suggested meaning for eps_writeMono()'s statement
c) my suggested meaning for the eps_writeMonoRGB()'s statement.


Cheers,
Jason

Whoops, I forgot to put a +1 and d+2 in part C about eps_writeMonoRGB(). It should say I think


has intended meaning



Cheers,
Jason

That was supposed to be like this.

I think I found an bug comm.c that affects com_readData() and com_readSubraster(). These two functions do not correctly propagate an ERR condition to the higher CDL (imd and cdl) functions. They incorrectly (as far as I see) require com_debug to be two or greater for this to happen. I don't see why we'd want to suppress it based on the debug value. An error is an error so we should propagate it regardless of com_debug's value.

For example, in com_readSubraster() the erroneous segment of code reads



This code occurs within a loop, and it's possible for a call to com_read() failure in a middle iteration of the loop to go unreported. What seems to be wanted here (and the idiom used in many places in comm.c) is



This has the advantage of eliminating the need for the status variable, which results in a 1% increase in speed too based on my testing, and being a tad easier to read.

Jason