nasa / bplib

Apache License 2.0
30 stars 13 forks source link

Suggest using type-safe wrappers for public API types #37

Closed jphickey closed 4 years ago

jphickey commented 4 years ago

Currently the API interface type is basically a void*:

https://github.com/nasa/bplib/blob/17d228c27ed93e1f3bf8b93091afa0319e3a6e28/inc/bplib.h#L146 https://github.com/nasa/bplib/blob/17d228c27ed93e1f3bf8b93091afa0319e3a6e28/inc/bplib.h#L162

The problem is that a void* is basically indistinguishable from any other pointer in the system. A user application could get these intermixed and pass anything (even a totally unrelated pointer) and the compiler will happy accept this without warning, as any pointer can implicitly become void* or vice versa.

It would add a degree of interface safety if these were at least wrapped in a struct, such as:

typedef struct {
    void *channel;
} bp_desc_t;

This way, a bp_desct_t can only be assigned or passed to another bp_desc_t, and any attempt do do anything else with it will generate a compiler type mismatch error.

Also recommend creating private (not exposed to the application API) methods to do the conversion between a bp_desc_t to/from a bp_channel_t*, such as:

static inline bp_channel_t *bplib_desc_to_channel(bp_desc_t desc)
{
    return (bp_channel_t *)desc.channel;
};

static inline bp_desc_t bplib_channel_to_desc(bp_channel_t *channel)
{
    bp_desc_t desc = { .channel = channel };
    return desc;
};

These converters could be used in place of the local casting.

jpswinski commented 4 years ago

Exposed that bp_desc_t is a pointer in the api; wrapped the void* type in a struct. Did not use the conversion functions. Made bp_sid_t unsigned long.