«

I'm writing a library (to communicate with a number of devices over a
serial port) and have some questions about the design. I have now a
header and source file like this:

/* device.h */
typedef struct device device;

int device_open (device **dev, const char *name);
int device_close (device *dev);
int device_read (device *dev, void *data, unsigned int size);
int device_write (device *dev, const void *data, unsigned int size);

/* device.c */

struct device {
/* some data here */
};

int
device_open (device **dev, const char *name)
{
if (dev == NULL)
return -1;

struct device *out = malloc (sizeof (struct device));
if (out == NULL)
return -1;

/* Do some stuff here */

*dev = out;
return 0;
}

As you can see, I made the internal data structure opaque by means of
the typedef and pointers to an incomplete data structure. Now my
questions are:

1. Is there any advantage to change the typedef to

typedef struct device *device;

and thus hiding the fact that the "device" type is actually a pointer.

2. I want to add support for a second type of device, which happens to
be very similar to the first one. For instance the read/write functions
are different, but the open/close functions and the contents of the
struct itself are the same. How can I implement this without copying the
code to a new pair of header and source files?

I was thinking to move the shared code to a common header and source
file and make the device-specific functions call the common functions.
But what should I do with the typdefs below, because both structs would
be the same in my case. But I would like to hide that from the user of
the library.

/* common.h (for internal use only) */

struct device {
/* some data here */
};

int device_open (device **dev, const char *name);

/* device1.h (public) */
typedef struct device1 device1;
int device1_open (device1 **dev, const char *name);
....

/* device2.h (public) */
typedef struct device2 device2;
int device1_open (device1 **dev, const char *name);
....

"Jef Driesen" <> wrote in message
> I'm writing a library (to communicate with a number of devices over a
> serial port) and have some questions about the design. I have now a
> header and source file like this:
>
> /* device.h */
> typedef struct device device;
>
> int device_open (device **dev, const char *name);
> int device_close (device *dev);
> int device_read (device *dev, void *data, unsigned int size);
> int device_write (device *dev, const void *data, unsigned int size);
>
Take a book out of the standard library.

FILE *fp = fopen("filename.txt", "w");
if(!fp)
/* it failed */
fprintf(fp, "This file works\n");
fclose(fp);

Similarly

typedef struct device DEVICE;

DEVICE *dev = device_open("Fred");
if(!dev)
/* your device didn't open */
device_write(dev, "my name is Fred", 15);
device_close(dev);

--
Free games and programming goodies.
http://www.personal.leeds.ac.uk/~bgy1mm

"Jef Driesen" <> wrote in message
news:woz%i.211246$...
> I'm writing a library (to communicate with a number of devices over a
> serial port) and have some questions about the design. I have now a header
> and source file like this:
>
> /* device.h */
> typedef struct device device;
>
> int device_open (device **dev, const char *name);
> int device_close (device *dev);
> int device_read (device *dev, void *data, unsigned int size);
> int device_write (device *dev, const void *data, unsigned int size);
>

device_open is ugly, IMO:
device *device_open (const char *name);
would be a little less ugly.


> /* device.c */
>
> struct device {
> /* some data here */
> };
>
> int
> device_open (device **dev, const char *name)
> {
> if (dev == NULL)
> return -1;
>
> struct device *out = malloc (sizeof (struct device));
> if (out == NULL)
> return -1;
>
> /* Do some stuff here */
>
> *dev = out;
> return 0;
> }
>
> As you can see, I made the internal data structure opaque by means of the
> typedef and pointers to an incomplete data structure. Now my questions
> are:
>
> 1. Is there any advantage to change the typedef to
>
> typedef struct device *device;
>
> and thus hiding the fact that the "device" type is actually a pointer.
>

this may be acceptable (I actually do similar with a void pointer at one
point):
typedef void *dyt;

where dyt hides that it is a pointer.
this is a personal preference though.

in my case, 'dyt' is a dynamic type for a specific 'dynamic type' API of
mine...


> 2. I want to add support for a second type of device, which happens to be
> very similar to the first one. For instance the read/write functions are
> different, but the open/close functions and the contents of the struct
> itself are the same. How can I implement this without copying the code to
> a new pair of header and source files?
>

a typical approach is to make use of structs filled with function pointers.
these exist per-type, and are referenced by the specific open instances.

struct device_info_s {
int (*close)(device *dev);
int (*read)(device *dev, void *data, unsigned int size);
int (*write)(device *dev, const void *data, unsigned int size);
};

the frontend functions then dispatch through this struct.

int device_close (device *dev)
{
if(dev->info->close)
return(dev->info->close(dev));
return(-1);
}

this allows plugging lots of different subsystems into the same interface,
and is especially useful, for example, in things like custom VFS subsystems,
....

for example, I did this at one point for a VFS API I called 'VFILE', and,
apart from the fact that I rewrote it at one point (the internals, the
frontend API remained more or less unchanged), this API has remained in use
for at least around a decade (this is pretty much some of the oldest code in
my projects, and one of my first 'formalized' APIs).

from what I remember, I originally wrote it long ago for an RDBMS type
project (this was back when I was in middle school or so I think...). (damn,
middle school was a long time ago...).


> I was thinking to move the shared code to a common header and source file
> and make the device-specific functions call the common functions. But what
> should I do with the typdefs below, because both structs would be the same
> in my case. But I would like to hide that from the user of the library.
>
> /* common.h (for internal use only) */
>
> struct device {
> /* some data here */
> };
>
> int device_open (device **dev, const char *name);
>
> /* device1.h (public) */
> typedef struct device1 device1;
> int device1_open (device1 **dev, const char *name);
> ...
>
> /* device2.h (public) */
> typedef struct device2 device2;
> int device1_open (device1 **dev, const char *name);
> ...


unnecessary API fragmentation is ugly, especially if you might expect more
of it in the future...

Malcolm McLean wrote:
> "Jef Driesen" <> wrote in message
>> I'm writing a library (to communicate with a number of devices over a
>> serial port) and have some questions about the design. I have now a
>> header and source file like this:
>>
>> /* device.h */
>> typedef struct device device;
>>
>> int device_open (device **dev, const char *name);
>> int device_close (device *dev);
>> int device_read (device *dev, void *data, unsigned int size);
>> int device_write (device *dev, const void *data, unsigned int size);
>>
> Take a book out of the standard library.
>
> FILE *fp = fopen("filename.txt", "w");
> if(!fp)
> /* it failed */
> fprintf(fp, "This file works\n");
> fclose(fp);
>
> Similarly
>
> typedef struct device DEVICE;
>
> DEVICE *dev = device_open("Fred");
> if(!dev)
> /* your device didn't open */
> device_write(dev, "my name is Fred", 15);
> device_close(dev);

My question was not about the discussion to return the pointer with the
return value or with an output parameter. I did choose the last option
to be able to return an error code directly. This also makes the error
handling of the open function similar to the other functions.

Jef Driesen wrote:
> I'm writing a library (to communicate with a number of devices over a
> serial port) and have some questions about the design. I have now a
> header and source file like this:
>
> /* device.h */
> typedef struct device device;
>
> int device_open (device **dev, const char *name);
> int device_close (device *dev);
> int device_read (device *dev, void *data, unsigned int size);
> int device_write (device *dev, const void *data, unsigned int size);
> [...]

> 1. Is there any advantage to change the typedef to
>
> typedef struct device *device;
>
> and thus hiding the fact that the "device" type is actually a pointer.

"Moving the star" from the parameter lists to the typedef
is perhaps an improvement, but a pretty small one. I think a
better course might be to leave the typedef as it is, and use
`device *device_open(const char *name)' with the convention that
NULL is returned on a failure. This idiom will be familiar to
all C programmers, following the existing model of fopen() and
the like -- but in order to follow it, you need to reveal the
pointer-nature of the returned value.

Keep in mind that hiding implementation details is a means,
not an end in itself. When you're wondering whether to hide
something, think about why you want to keep it hidden.

> 2. I want to add support for a second type of device, which happens to
> be very similar to the first one. For instance the read/write functions
> are different, but the open/close functions and the contents of the
> struct itself are the same. How can I implement this without copying the
> code to a new pair of header and source files?

You could maintain a type indicator in the `struct device'.
One particularly flexible indicator is a set of function pointers:

struct device {
...
int (*read)(device*, void*, unsigned int);
int (*write)(device*, void*, unsigned int);
...
};

If there are a lot of such pointers, you might want to use one
more level of indirection:

struct funcs {
int (*read)(device*, void*, unsigned int);
int (*write)(device*, void*, unsigned int);
int (*scribble)(device*, void*, unsigned int);
int (*doodle)(device*, void*, unsigned int);
int (*spray)(device*, PaintColor);
};

static const struct funcs type1 = {
read1, write1, scribble1, doodle1, sprayany };
static const struct funcs type2 = {
read2, write2, scribble2, doodle2, sprayany };

struct device {
...
const struct funcs *funcs; /* to type1 or type2 */
...
};

As an aside, why use `unsigned int' for what appear to be
counts, instead of `size_t'? That's what it's for, after all.

--
Eric Sosman
lid

On Sat, 17 Nov 2007 21:13:40 +1000, "cr88192" wrote:
>"Jef Driesen" <> wrote in message
>> I'm writing a library (to communicate with a number of devices over a
>> serial port) and have some questions about the design. I have now a header
>> and source file like this:
>>
>> /* device.h */
>> typedef struct device device;
>>
>> int device_open (device **dev, const char *name);
>> int device_close (device *dev);
>> int device_read (device *dev, void *data, unsigned int size);
>> int device_write (device *dev, const void *data, unsigned int size);
>
>device_open is ugly, IMO:
>device *device_open (const char *name);
>would be a little less ugly.

alternatively:

struct device_handle {
device* imp;
} device_handle;
typedef struct device_handle device_handle;

device_handle device_open (const char *name);

>> 2. I want to add support for a second type of device, which happens to be
>> very similar to the first one. For instance the read/write functions are
>> different, but the open/close functions and the contents of the struct
>> itself are the same. How can I implement this without copying the code to
>> a new pair of header and source files?
>
>a typical approach is to make use of structs filled with function pointers.
>these exist per-type, and are referenced by the specific open instances.

or just a 'switch on type' which invisible for the user, too.


--
Roland Pibinger
"The best software is simple, elegant, and full of drama" - Grady Booch

On Sat, 17 Nov 2007 18:08:34 GMT, Roland Pibinger wrote:

>alternatively:
>
>struct device_handle {
> device* imp;
>} device_handle;
>typedef struct device_handle device_handle;

Should have been:

struct device_handle {
device* imp;
};
typedef struct device_handle device_handle;

"cr88192" <> wrote in message
news:bb0c7$473ecd70$ca8010a3$...
>
> "Jef Driesen" <> wrote in message
[...]

>> 2. I want to add support for a second type of device, which happens to be
>> very similar to the first one. For instance the read/write functions are
>> different, but the open/close functions and the contents of the struct
>> itself are the same. How can I implement this without copying the code to
>> a new pair of header and source files?
>>
>
> a typical approach is to make use of structs filled with function
> pointers.
> these exist per-type, and are referenced by the specific open instances.
>
> struct device_info_s {
> int (*close)(device *dev);
> int (*read)(device *dev, void *data, unsigned int size);
> int (*write)(device *dev, const void *data, unsigned int size);
> };
>
> the frontend functions then dispatch through this struct.
>
> int device_close (device *dev)
> {
> if(dev->info->close)
> return(dev->info->close(dev));
> return(-1);
> }
>
> this allows plugging lots of different subsystems into the same interface,
> and is especially useful, for example, in things like custom VFS
> subsystems, ...
[...]

FWIW, here is a way to do minimalist interfaces in C:

http://groups.google.com/group/comp....106926ba5db19f

"Chris Thomasson" <> wrote in message
news:. ..
> "cr88192" <> wrote in message
> news:bb0c7$473ecd70$ca8010a3$...
>>
>> "Jef Driesen" <> wrote in message
> [...]
>
>>> 2. I want to add support for a second type of device, which happens to
>>> be very similar to the first one. For instance the read/write functions
>>> are different, but the open/close functions and the contents of the
>>> struct itself are the same. How can I implement this without copying the
>>> code to a new pair of header and source files?
>>>
>>
>> a typical approach is to make use of structs filled with function
>> pointers.
>> these exist per-type, and are referenced by the specific open instances.
>>
>> struct device_info_s {
>> int (*close)(device *dev);
>> int (*read)(device *dev, void *data, unsigned int size);
>> int (*write)(device *dev, const void *data, unsigned int size);
>> };
>>
>> the frontend functions then dispatch through this struct.
>>
>> int device_close (device *dev)
>> {
>> if(dev->info->close)
>> return(dev->info->close(dev));
>> return(-1);
>> }
>>
>> this allows plugging lots of different subsystems into the same
>> interface, and is especially useful, for example, in things like custom
>> VFS subsystems, ...
> [...]
>
> FWIW, here is a way to do minimalist interfaces in C:
>
> http://groups.google.com/group/comp....106926ba5db19f
>
>

yes.
this does not seem too much different from what I was suggesting here.


this is one way to do OO/abstract APIs.
actually, often in such APIs (my case), one can recognize that there are
often multiple paths to the same result, so, trying to use one method, if it
is not present, the wrapper may look into a few other possibilities
(fallback cases), before finally returning error.

an example in my 'VFILE' API (ok, actually for the most part a shameless rip
of stdio, as nearly everything is the same except for a 'V' or 'v' prefix on
everything, though in a few cases I used 'vf' as well, or used 'vf' as a
suffix...).

this was done so that it would be really easy to convert code to/from the
stdio file interface (though slightly limited, as this idea was developed
before I really discovered search-and-replace code-conversion, which would
mandate a slightly different naming convention to make work well, but
search-replace followed by more search-replace to deal with conversion
issues often works good enough...).

or such...


>
>

On Sat, 17 Nov 2007 10:29:48 GMT, Jef Driesen
<> wrote:

>I'm writing a library (to communicate with a number of devices over a
>serial port) and have some questions about the design. I have now a
>header and source file like this:
>
>/* device.h */
>typedef struct device device;
>
>int device_open (device **dev, const char *name);
>int device_close (device *dev);
>int device_read (device *dev, void *data, unsigned int size);
>int device_write (device *dev, const void *data, unsigned int size);
>
>/* device.c */
>
>struct device {
> /* some data here */
>};
>
>int
>device_open (device **dev, const char *name)
>{
> if (dev == NULL)
> return -1;
>
> struct device *out = malloc (sizeof (struct device));
> if (out == NULL)
> return -1;
>
> /* Do some stuff here */
>
> *dev = out;
> return 0;
>}
>
>As you can see, I made the internal data structure opaque by means of
>the typedef and pointers to an incomplete data structure. Now my
>questions are:
>
>1. Is there any advantage to change the typedef to
>
>typedef struct device *device;

IMO, no advantage and a major disadvantage. If you make the change,
then code declaring a pointer will look like
device name;
which will inevitably at some point in the future lead some
unsuspecting maintenance programmer to believe he needs to add an &
when he passes name to some function expecting a pointer.

If you still decide to do it, change the typedef name from *device to
something like *device_ptr_t to give this unfortunate heir a hint.

>
>and thus hiding the fact that the "device" type is actually a pointer.

Hide only the details that are worth hiding. As has already been
suggested, FILE is a precedent worth following.

>
>2. I want to add support for a second type of device, which happens to
>be very similar to the first one. For instance the read/write functions
>are different, but the open/close functions and the contents of the
>struct itself are the same. How can I implement this without copying the
>code to a new pair of header and source files?

Again, FILE and the standard library show a reasonable approach. For
the things that are common, such as the struct declaration and the
open and close functions, you only need one. For the things that are
specific to a particular device (read and write functions), you need
unique versions. But the prototypes for these unique functions can
(not must) still be in a common header.

On my system, the standard headers include both the public and the
opaque information. You can't really hide the opaque data from the
programmer since he has to be able to read the headers to perform the
compile. It becomes a question of how much he has to wade through to
find the information.

If one program is capable of dealing with two device types, you have
the risk of a common structure associated with the second device being
passed to a function dedicated to the first device. Two typedef's
aliasing the same struct type will not solve this problem. If you
include in the structure a member indicating which of the possible
devices this instance of the structure applies to, your device
dependent routines can check before they do any real work.

>
>I was thinking to move the shared code to a common header and source
>file and make the device-specific functions call the common functions.

Putting code in a header is almost always a mistake. Maybe that's not
what you meant.

>But what should I do with the typdefs below, because both structs would
>be the same in my case. But I would like to hide that from the user of
>the library.
>
>/* common.h (for internal use only) */
>
>struct device {
> /* some data here */
>};
>
>int device_open (device **dev, const char *name);
>
>/* device1.h (public) */
>typedef struct device1 device1;
>int device1_open (device1 **dev, const char *name);
>...
>
>/* device2.h (public) */
>typedef struct device2 device2;
>int device1_open (device1 **dev, const char *name);
>...

A single header: device.h

typedef struct device
{
...
enum type {device_1, device_2, ...};
...
} device;
device* device_open(...);
device* device_close(...);
int device_1_read(...);
...

A source file for each function, such as device_open.c

#include <device.h>
#include <stdlib.h>
device* device_open(...)
{
device *ptr = malloc(...);
if (ptr != NULL)
{
...
}
return ptr;
{

In device dependent functions, such as device_1_read

int device_1_read(device *ptr, ......)
{
if (ptr->type != device_1)
/*error handler*/
...
}

Whenever a device is opened

dev = device_open(...);
if (dev == NULL)
/* error handler */
dev_type = device_1; /*or _2 or whatever */


Remove del for email