Idiomatic module encapsulation

[Noob]

Hello everyone,

I'm implementing a wrapper around platform-dependent "primitives",
and I'm trying to have 0 platform-dependent code (even includes)
in the "module" interface header.

For example, consider this possible interface for a semaphore
(whatever that is):

Sem_t *Sem_create(int val);
void Sem_delete(Sem_t *sem);
int Sem_signal(Sem_t *sem);
int Sem_wait(Sem_t *sem);
int Sem_wait_timeout(Sem_t *sem, unsigned wait_ms);

I could use an opaque (void *) pointer to "hide" the details of
a (Sem_t) but I do want the compiler to throw an error when an
imprudent user calls, e.g.

char *str = "booya";
Sem_signal(str);

One method I've seen a few times, is to "lie" to the user code
(and, to some level, to the compiler) by pretending "yeah, this
Sem_t is a struct, and I'll provide the definition sometime later"
(I think it's called a tentative declaration). But I've been told
that the compiler always gets its revenge when it's lied to.

Could you please confirm whether the following approach is safe,
and does what I want?

$ cat wrap_sem.h
#ifndef TRUE_SEM_T_DEFINITION
typedef struct dummy_Sem_t Sem_t;
#endif
Sem_t *Sem_create(int val);
void Sem_delete(Sem_t *sem);
int Sem_signal(Sem_t *sem);
int Sem_wait(Sem_t *sem);
int Sem_wait_timeout(Sem_t *sem, unsigned wait_ms);

Users just include "wrap_sem.h" and they're not supposed to "peek"
inside a Sem_t (in fact, they can't since Sem_t is an incomplete type).

$ cat test.c
#include "wrap_sem.h"
int main(void)
{
Sem_t *toto = Sem_create(42);
*toto;
Sem_wait("foo");
return 0;
}

$ gcc -Wall -Wextra -std=c89 test.c
test.c: In function 'main':
test.c:5:3: error: dereferencing pointer to incomplete type
test.c:6:3: warning: passing argument 1 of 'Sem_wait' from incompatible pointer type
wrap_sem.h:7:5: note: expected 'struct Sem_t *' but argument is of type 'char *'

(Side issue: there seems to be a small QoI issue with the compiler's note,
as there is no 'struct Sem_t' AFAIU, there is a 'struct dummy_Sem_t' and
a 'Sem_t'. Do you agree? End digression)

In the actual implementation file, I define TRUE_SEM_T_DEFINITION
and then define the "real" Sem_t type.

Is this an (the?) idiomatic way to hide implementation details?
Is there a chance that this could bite me?
Could this confuse other tools, such as a debugger?
Any unforeseen consequences?

Regards.

[Noob]

In fact, the compiler does not complain if TRUE_SEM_T_DEFINITION
is never defined. In the implementation, I just work with the
"real" type, say sem_t.

e.g.

$ cat wrap_sem.c
#include <semaphore.h>

int Sem_signal(sem_t *sem)
{
return sem_post(sem);
}

int Sem_wait(sem_t *sem)
{
return sem_wait(sem);
}

I'm confused. How can the compiler be happy when I said
"there exists a Sem_t type that is the same as a struct that
doesn't exist, and there are these functions that return a
pointer (or take one) to such a beast;" but when I implement
the functions, they take a pointer to a different kind of
structure?

For example:

typedef struct dummy_Sem_t Sem_t;
int Sem_signal(Sem_t *sem);
int Sem_signal(sem_t *sem)
{
return sem_post(sem);
}

( considering int sem_post(sem_t *sem); )

Are Sem_t and sem_t compatible by accident?
Because of the specific definition of sem_t?

/me confused.

[Noob]

Please ignore my second message(*) [which I cancelled, in case
a few servers do not ignore such requests]

(*) Message-ID: <jpfmvh$4ut$1...@dont-email.me>

My actual code looks like this:

$ cat wrap_sem.h
#ifndef H_WRAP_SEM
#define H_WRAP_SEM

#ifndef TRUE_SEM_T_DEFINITION
typedef struct dummy_Sem_t Sem_t;
#endif
Sem_t *Sem_create(int val);
void Sem_delete(Sem_t *sem);
int Sem_signal(Sem_t *sem);
int Sem_wait(Sem_t *sem);

int Sem_wait_timeout(Sem_t *sem, unsigned timeout_ms);

#endif /* H_WRAP_SEM */

$ cat wrap_sem.c
#include <semaphore.h>

#define TRUE_SEM_T_DEFINITION
typedef sem_t Sem_t;
#include "wrap_sem.h"
sem_t *Sem_create(int val)
{
return NULL;
}

void Sem_delete(sem_t *sem)
{
return NULL;

}

int Sem_signal(sem_t *sem)
{
return sem_post(sem);
}

int Sem_wait(sem_t *sem)
{
return sem_wait(sem);
}

Regards.

[James Kuyper]

On 05/22/2012 05:18 AM, Noob wrote:
> Hello everyone,
>
> I'm implementing a wrapper around platform-dependent "primitives",
> and I'm trying to have 0 platform-dependent code (even includes)
> in the "module" interface header.
>
> For example, consider this possible interface for a semaphore
> (whatever that is):

IIRC, some standard other than the C standard (I think it's POSIX or one
it's descendent's) reserves all identifiers ending in _t for use as type
names associated with the standard libraries. You might want to change
the name of Sem_t to avoid conflicts.

> Sem_t *Sem_create(int val);
> void Sem_delete(Sem_t *sem);
> int Sem_signal(Sem_t *sem);
> int Sem_wait(Sem_t *sem);
> int Sem_wait_timeout(Sem_t *sem, unsigned wait_ms);
>
> I could use an opaque (void *) pointer to "hide" the details of
> a (Sem_t) but I do want the compiler to throw an error when an
> imprudent user calls, e.g.
>
> char *str = "booya";
> Sem_signal(str);
>
> One method I've seen a few times, is to "lie" to the user code
> (and, to some level, to the compiler) by pretending "yeah, this
> Sem_t is a struct, and I'll provide the definition sometime later"
> (I think it's called a tentative declaration). But I've been told
> that the compiler always gets its revenge when it's lied to.

There's an easy solution to that: don't lie. In the public header,
declare your struct type without a definition. In a private header
shared only by your implementation of those functions, define the same
exact struct type as containing the implementation-dependent features it
needs to contain, and define your functions as taking and/or returning
pointers to that struct type.

> Could you please confirm whether the following approach is safe,
> and does what I want?
>
> $ cat wrap_sem.h
> #ifndef TRUE_SEM_T_DEFINITION
> typedef struct dummy_Sem_t Sem_t;
> #endif
> Sem_t *Sem_create(int val);
> void Sem_delete(Sem_t *sem);
> int Sem_signal(Sem_t *sem);
> int Sem_wait(Sem_t *sem);
> int Sem_wait_timeout(Sem_t *sem, unsigned wait_ms);
>
> Users just include "wrap_sem.h" and they're not supposed to "peek"
> inside a Sem_t (in fact, they can't since Sem_t is an incomplete type).
>
> $ cat test.c
> #include "wrap_sem.h"
> int main(void)
> {
> Sem_t *toto = Sem_create(42);
> *toto;
> Sem_wait("foo");
> return 0;
> }
>
> $ gcc -Wall -Wextra -std=c89 test.c
> test.c: In function 'main':
> test.c:5:3: error: dereferencing pointer to incomplete type

For a proper opaque type, the user must never dereference the pointer.
This is not normally much of a problem, because the user seldom has need
to dereference it. The main exception is copying; if you need the user
to have some means of copying objects of the opaque type, you need to
provide a copying function. In the simplest case:

Sem_t *Sem_copy(const Sem_t *source)
{
Sem_t *dest = malloc(sizeof *dest);
if(dest)
*dest = *source;
return dest;
}

You'll need an object factory: a function that returns a pointer to an
newly allocated and appropriately initialized object of the opaque type.
You'll also need to let the users know that they should free() the
pointers when they're done with them.

In more complex cases, if Sem_t itself contains pointers, the pointed-at
objects might need to be copied too, and you might need to define a
special function for destroying such objects that free()s the pointed-at
memory.

> test.c:6:3: warning: passing argument 1 of 'Sem_wait' from incompatible pointer type
> wrap_sem.h:7:5: note: expected 'struct Sem_t *' but argument is of type 'char *'
>
> (Side issue: there seems to be a small QoI issue with the compiler's note,
> as there is no 'struct Sem_t' AFAIU, there is a 'struct dummy_Sem_t' and
> a 'Sem_t'. Do you agree? End digression)
>
> In the actual implementation file, I define TRUE_SEM_T_DEFINITION
> and then define the "real" Sem_t type.

Drop that. It should have the same definition in both places. Just
declare struct dummy_Sem_t as containing an object of the type that you
wanted to use for Sem_t.

> Is this an (the?) idiomatic way to hide implementation details?

No. The alternative I've described is the idiomatic way.

> Is there a chance that this could bite me?

Yes. Your approach has undefined behavior, because the declaration of
your functions that's visible to users is incompatible with the
definitions of those same functions.

> Could this confuse other tools, such as a debugger?

Quite likely.

> Any unforeseen consequences?
Almost certainly.
--
James Kuyper

[August Karlstrom]

On 2012-05-22 11:18, Noob wrote:
> Hello everyone,
>
> I'm implementing a wrapper around platform-dependent "primitives",
> and I'm trying to have 0 platform-dependent code (even includes)
> in the "module" interface header.
>
> For example, consider this possible interface for a semaphore
> (whatever that is):
>
> Sem_t *Sem_create(int val);
> void Sem_delete(Sem_t *sem);
> int Sem_signal(Sem_t *sem);
> int Sem_wait(Sem_t *sem);
> int Sem_wait_timeout(Sem_t *sem, unsigned wait_ms);
>
> I could use an opaque (void *) pointer to "hide" the details of
> a (Sem_t) but I do want the compiler to throw an error when an
> imprudent user calls, e.g.
>
> char *str = "booya";
> Sem_signal(str);
>
> One method I've seen a few times, is to "lie" to the user code
> (and, to some level, to the compiler) by pretending "yeah, this
> Sem_t is a struct, and I'll provide the definition sometime later"
> (I think it's called a tentative declaration). But I've been told
> that the compiler always gets its revenge when it's lied to.

[...]

As far as I know the idiomatic way do define an abstract data type in C
is to split the declarations and definitions like in the following.

In semaphore.h:

struct semaphore;

struct semaphore *sem_create(int val);
...


In semaphore.c:

#include "semaphore.h"

struct semaphore {
...
};

struct semaphore *sem_create(int val)
{
...
}


/August

[Noob]

James Kuyper wrote:

> Noob wrote:
>
>> I'm implementing a wrapper around platform-dependent "primitives",
>> and I'm trying to have 0 platform-dependent code (even includes)
>> in the "module" interface header.
>>
>> For example, consider this possible interface for a semaphore
>> (whatever that is):
>
> IIRC, some standard other than the C standard (I think it's POSIX or one

> its descendents) reserves all identifiers ending in _t for use as type

> names associated with the standard libraries. You might want to change
> the name of Sem_t to avoid conflicts.

Thanks for pointing that out, it hadn't crossed my mind.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02_02

>> I could use an opaque (void *) pointer to "hide" the details of
>> a (Sem_t) but I do want the compiler to throw an error when an
>> imprudent user calls, e.g.
>>
>> char *str = "booya";
>> Sem_signal(str);
>>
>> One method I've seen a few times, is to "lie" to the user code
>> (and, to some level, to the compiler) by pretending "yeah, this
>> Sem_t is a struct, and I'll provide the definition sometime later"
>> (I think it's called a tentative declaration). But I've been told
>> that the compiler always gets its revenge when it's lied to.
>
> There's an easy solution to that: don't lie. In the public header,
> declare your struct type without a definition. In a private header
> shared only by your implementation of those functions, define the same
> exact struct type as containing the implementation-dependent features it
> needs to contain, and define your functions as taking and/or returning
> pointers to that struct type.

What if my 'Sem_t' is, in fact, NOT a struct?
For example, on POSIX platforms, I have:
typedef sem_t Sem_t;
(sem_t /itself/ is an implementation-defined "opaque" type)

>> Could you please confirm whether the following approach is safe,
>> and does what I want?
>>
>> $ cat wrap_sem.h
>> #ifndef TRUE_SEM_T_DEFINITION
>> typedef struct dummy_Sem_t Sem_t;
>> #endif
>> Sem_t *Sem_create(int val);
>> void Sem_delete(Sem_t *sem);
>> int Sem_signal(Sem_t *sem);
>> int Sem_wait(Sem_t *sem);
>> int Sem_wait_timeout(Sem_t *sem, unsigned wait_ms);
>>
>> Users just include "wrap_sem.h" and they're not supposed to "peek"
>> inside a Sem_t (in fact, they can't since Sem_t is an incomplete type).
>>

>> In the actual implementation file, I define TRUE_SEM_T_DEFINITION
>> and then define the "real" Sem_t type.
>
> Drop that. It should have the same definition in both places. Just
> declare struct dummy_Sem_t as containing an object of the type that you
> wanted to use for Sem_t.
>

>> Is there a chance that this could bite me?
>
> Yes. Your approach has undefined behavior, because the declaration of
> your functions that's visible to users is incompatible with the
> definitions of those same functions.

I had not thought of using a struct with a single member.
It makes the code slightly less legible, but I hid the artificial
address-of-dereferenced-member behind a macro.

My current solution:

$ cat wrap_sem.h
#ifndef H_WRAP_SEM
#define H_WRAP_SEM

typedef struct Sem_t Sem_t;

Sem_t *Sem_create(int val);
int Sem_delete(Sem_t *sem);

int Sem_signal(Sem_t *sem);
int Sem_wait(Sem_t *sem);

int Sem_wait_timeout(Sem_t *sem, unsigned timeout_ms);

#endif /* H_WRAP_SEM */

$ cat wrap_sem.c
#include <semaphore.h>

#include <stdlib.h>
#include "wrap_sem.h"

struct Sem_t
{
sem_t sem;
};

#define sem &wrap->sem
#define RESET(p) do { free(p); p = NULL; } while ( 0 )

Sem_t *Sem_create(int val)
{
Sem_t *wrap = malloc(sizeof *wrap);
if (wrap != NULL)
{
int err = sem_init(sem, 0, val);
if (err) RESET(wrap);
}
return wrap;
}

int Sem_delete(Sem_t *wrap)
{
int err = 0;
if (wrap != NULL)
{
err = sem_destroy(sem);
free(wrap);
}
return err;
}

int Sem_signal(Sem_t *wrap)
{
return sem_post(sem);
}

int Sem_wait(Sem_t *wrap)
{
return sem_wait(sem);
}

int Sem_wait_timeout(Sem_t *wrap, unsigned timeout_ms)
{
unsigned sec = timeout_ms / 1000;
unsigned msec = timeout_ms - 1000*sec;
struct timespec spec = { sec, 1000000*msec };
return sem_timedwait(sem, &spec);
}

This is all standard-compliant, right?
(Except initialization of 'spec' not allowed in C89.)
No nasties waiting by the road :-)

$ gcc -Wall -Wextra -std=c89 -pedantic -Os -S wrap_sem.c
wrap_sem.c: In function 'Sem_wait_timeout':
wrap_sem.c:49:10: warning: initializer element is not computable at load time
wrap_sem.c:49:10: warning: initializer element is not computable at load time

Regards.

[James Kuyper]

On 05/22/2012 10:09 AM, Noob wrote:
> James Kuyper wrote:
...

>> There's an easy solution to that: don't lie. In the public header,
>> declare your struct type without a definition. In a private header
>> shared only by your implementation of those functions, define the same
>> exact struct type as containing the implementation-dependent features it
>> needs to contain, and define your functions as taking and/or returning
>> pointers to that struct type.
>
> What if my 'Sem_t' is, in fact, NOT a struct?
> For example, on POSIX platforms, I have:
> typedef sem_t Sem_t;
> (sem_t /itself/ is an implementation-defined "opaque" type)

That could work, so long as you only work with pointers to Sem_t.
However, if you ever need to create actual Sem_t objects, there's
potential problems. If sem_t were in fact an opaque type you would not
be able to define an object of that type, only a pointer to it, in which
case you would have to do something like:

struct dummy_Sem_t{
sem_t *sem;
}

However, sem_t is merely a "translucent" type: sem_t is defined in
<semaphore.h>, and not merely declared there. You shouldn't write code
depending upon the details of how it's defined, but the presence of that
definition allows the more direct approach you used below:

> struct Sem_t
> {
> sem_t sem;
> };

I wouldn't bother wrapping this with a macro, I think it's better to
leave this kind of detail unwrapped:

> #define sem &wrap->sem

However, if you do choose to use it, I recommend following conventional
used by giving it a name in all CAPS. Otherwise maintenance programmers
are going to be wondering where they can find this variable named 'dem'.

> #define RESET(p) do { free(p); p = NULL; } while ( 0 )
>
> Sem_t *Sem_create(int val)
> {
> Sem_t *wrap = malloc(sizeof *wrap);
> if (wrap != NULL)
> {
> int err = sem_init(sem, 0, val);
> if (err) RESET(wrap);
> }
> return wrap;
> }
>
> int Sem_delete(Sem_t *wrap)
> {
> int err = 0;
> if (wrap != NULL)
> {
> err = sem_destroy(sem);
> free(wrap);
> }
> return err;
> }
>
> int Sem_signal(Sem_t *wrap)
> {
> return sem_post(sem);
> }
>
> int Sem_wait(Sem_t *wrap)
> {
> return sem_wait(sem);
> }
>
> int Sem_wait_timeout(Sem_t *wrap, unsigned timeout_ms)
> {
> unsigned sec = timeout_ms / 1000;
> unsigned msec = timeout_ms - 1000*sec;
> struct timespec spec = { sec, 1000000*msec };
> return sem_timedwait(sem, &spec);
> }
>
> This is all standard-compliant, right?
> (Except initialization of 'spec' not allowed in C89.)

I don't see any obvious problems, but I'm not as reliable as I'd like to
be when looking for such things.

[Noob]

James Kuyper wrote:

> I don't see any obvious problems, but I'm not as reliable as I'd like
> to be when looking for such things.

Thanks for the insight and advice! (Thanks August too.)

Regards.

[Joe keane]

In article <jpfllp$tsu$1...@dont-email.me>, Noob <ro...@127.0.0.1> wrote:
>Sem_t *Sem_create(int val);
>void Sem_delete(Sem_t *sem);

int Sem_create(int val, Sem_t **ret);
int Sem_delete(Sem_t *sem);

[Noob]

Joe keane wrote:

> Noob wrote:
>
>> Sem_t *Sem_create(int val);
>> void Sem_delete(Sem_t *sem);
>
> int Sem_create(int val, Sem_t **ret);
> int Sem_delete(Sem_t *sem);

I see your point.

But I didn't choose the APIs, they were set in stone...

[Robert Miles]

On 5/22/2012 5:08 AM, Noob wrote:
> Noob wrote:
[snip]

>> Is this an (the?) idiomatic way to hide implementation details?
>> Is there a chance that this could bite me?
>> Could this confuse other tools, such as a debugger?
>> Any unforeseen consequences?
>
> Please ignore my second message(*) [which I cancelled, in case
> a few servers do not ignore such requests]

I'd restate that as most newsgroups servers now ignore cancel
requests. See here for why:

http://whatis.techtarget.com/definition/cancelbot

Cancelbots became much too common several years ago, and were
often used to cancels all posts by people that the person
running the cancelbot didn't like.