maedoc / libtvb

TVB C library
6 stars 8 forks source link

Consistent API for all objects #90

Open maedoc opened 8 years ago

maedoc commented 8 years ago
typedef struct sd_obj { 
    void *ptr;
    void     (* const   free)(sd_obj*);
    uint32_t (* const nbytes)(sd_obj*);
    sd_stat  (* const  write)(sd_obj*, sd_writer*);
} sd_obj;

sd_obj * sd_obj_read(sd_reader*);
maedoc commented 8 years ago

I/O or serialization (read, write) is a tricky subject I would avoid for now

AbheekG commented 8 years ago

@maedoc would like to fix this one. The nbytes counting will be better if done in sd_*alloc part or in the object.

maedoc commented 8 years ago

@abheekg OK, just focus on adding methods for nbytes to each sd_* struct. There may be some tricky cases like a double * where the size isn't obvious. One example is hist_dat.buf. You're welcome to leave those out and let me fill it it.

One way to test nbytes method would be add something like a sd_malloc_total_nbytes function which returns the total nbytes allocated by all calls to sd_malloc. Then you can

uint32_t tic = sd_malloc_total_nbytes();
sd_foo * foo = sd_foo_new();
EXPECT_EQ(foo->nbytes(foo) + tic), sd_malloc_total_nbytes());

to test that the sd_foo->nbytes method is correct. Can you also add those tests to the existing tests? I can take a look at the test failures to see what details might be tricky.

AbheekG commented 8 years ago

@maedoc In sd_hist, the pointers lim, len, pos are of type uint32_t, but during allocation are allocated by double. Is it a bug or ..?

maedoc commented 8 years ago

Good catch of my copy pasting :) a double is bigger than a uint32_t so no potential memory corruption, but yes those should be uint32 allocations.

I wonder why the compiler didnt mention it..

maedoc commented 8 years ago

I meant that GCC sometimes can warn you if the type argument to sizeof does not match the l value pointer type

AbheekG commented 8 years ago

For sd_malloc_total_nbytes(). If we take realloc into consideration, individual memory size will also have to be stored. We can use the same object as malloc registered pointers #107 . Then we will have to activate mem. registration by default. Please if you can review the registered pointer allocation, or I should ignore this, sd_malloc_total_nbytes() only counting additions.

maedoc commented 8 years ago

I will review the other pull request later today. Can you make sure your most recent commits are push to that PR?