Why not deal with stack-allocated calibration object instead of heap-allocated?

[acorbe]

In the calibration.{c,h} files, the final calibration object is currently instantiated during the call read_calibration() (below) and owned by the caller via a pointer.

 Calibration *read_calibration(char *ori_file, char *add_file, char *fallback_file);

Since the whole calibration struct is fully stack-like allocated, why not give full ownership to the caller and have something like

int read_calibration(Calibration * calibration,char *ori_file, char *add_file, char *fallback_file);

where int is a return flag.

Moreover, this way
. there is no ambiguity on the way the calibration object has to be deallocated
. faulty calls which (for some misterious reasons) don't capture the returned value do not end up in memory leaks

[yosefm]

I have no objection to making this change. Currently the calibration object is only used in the tests, since I've put work on it on hold for a long time, so the switch would be easy.

However, the same allocation method is used in parameters.c and this is already used in several places on openptv-python and would require patching.

[alexlib]

I agree that this can be changed. Can we track down all the places where we'll need to update the Cython bindings due to this change?

[yosefm]

Any IDE's code search option can do it. Personally I prefer ack-grep. It seems like for the parameters structs the only current uses are (mostly) not in the python bindings but in the C code. It's not a lot of work to change the instantiation, but adding a stack variable and passing a reference to it instead of the pointer used right now will be some grunt work. That's all.

[acorbe]

Hi all,
maybe I've been too precipitous.
Let me propose a more refined idea which goes in the direction of easing the uniform heap-allocation (as the code is at the moment).

Calibration does not contain any pointer, hence it can be stack allocated without pains.
On the other hand, there are structs like control_par which do contain pointers and hence require de-allocation of the objects they point to (see free_control_par which de-allocates member first and then the structure itself).

Making Calibration preferably stack-allocated, an asymmetry with respect to (de)allocation policies of other structures would rise.

So here is the proposal (somehow inspired by petsc, not involving many modification, and probably to be further elaborated):

1. Every structure is used through a reference, e.g.

     typedef struct Calibration* rCalibration;   //to prevent hybrid stack and heap uses. Heap use is default.
   

2. Structures are initialized via functions like

     int readCalibration( rCalibration * in , char * filename );   //reference to calibration is passed by reference to allocate
   

3. Structures are used like

     void raytrace(double* out, double in*, rCalibration calib);   //reference to calibration is passed by value for use
   

4. Structure are deallocated via

    int destroy_calibration(rCalibration calib); // or, more safely, int destroy_calibration(rCalibration* calib);  
   

which is there for every structure (also for the simplest ones and in such cases just wraps the free function).

Indeed, a reduction of such proposal which just focuses on 4 and then 1 is possible:

I would keep 4. so to have a uniform de-allocation policy based on specific free-like functions (which can possibly just forward the call to free).

I would keep 1. so to make clear that
. "fundamental" types are references-like (as Calibration*)
. allocation is fully-heap and any other use is discouraged.

Points 2 and 3 can be cast in the current design
2. rCalibration readCalibration( char * filename ); //i.e. no mod required
3. modifications are obvious

What do you think?

[yosefm]

1 and 2 are acceptable. Then whenever possible we make it the user's responsibility to allocate. However, we might as well prevent errors and misuse of malloc() and free() by hiding them in the reading/destroying functions. A nice, convenient compromise would be to use your method in (2), but allow to pass in a NULL pointer, in which case the reader would allocate a new object on the heap. This is just one mere 'if' statement.

4 Is a good idea, and actually we also miss free_sequence_par() which is possibly leaky. The fact that we don't read these files many times hides it. Only change I would suggest is using free_* instead of destroy_* because that's what we started doing in parameters.c

As for 3, I'd rather leave that as individual decision for each function. If the structures are not touched, pass by reference saves a copy. If it is touced, and we can't get rid of this touching, may be better to pass by value, then discard the copy.