0

I got the following code:

// file external_module.h
typedef externaldata * externalhdl; // opaque pointer
externalhdl external_Create();

// file internal_module.h
typedef internaldata * internalhdl; // opaque pointer
internalhdl internal_Create();

What i would like to do is to use an opaque pointer of a external module as a opaque pointer inside of my module to save unessasary allocs. Currently my workaround implimentation is:

typedef struct {externalhdl e} internaldata;

internalhdl internal_Create()
{
    internalhdl p = (internaldata*) malloc (sizeof(internaldata));
    p.e = external_Create();
    return p;
}

What I would like to do is use something like:

typedef ??? internaldata; //Don't know how 

internalhdl internal_Create()
{
    return external_Create();
}

From my point of view it should be possible since both are pointer but I need to get it warning free? Thanks for your help.

Schafwolle
  • 501
  • 3
  • 15
  • Why dont you create a header file, where you can define this struct, and include this file into all your source? – betontalpfa Mar 02 '17 at 13:23
  • 8
    No, you do **not** want to do this! You do not want to `typedef` an opject pointer at all! To be most clear: **never ever** `typedef` a pointer to a data type! Oh, and `typedef` has absolutely noting to do with memory allocation. And provide a [mcve]. Your code structure and your problem are not clear. From what you show it looks as if you are lost in your personal `typedef` hell. – too honest for this site Mar 02 '17 at 13:25
  • 1
    @betontalpfa: Beacuse that't the idea behind _opaque_ types. Just that it should not be the pointer being `typedef`ed. – too honest for this site Mar 02 '17 at 13:26
  • FYI: `typedef` in C has not the same implications as in other languages, i.e. it does not create a new type. It is just an alias. You do not gain any advantage for type-checking. – too honest for this site Mar 02 '17 at 13:39
  • Also, it doesn't matter what type the pointer is, you don't need to cast `malloc()`, `void *` is converted to any pointer type without casting. And typedefing a pointer could introduce subtle errors like `my_pointer_type p = malloc(sizeof(my_pointer_type))` when you really mean `my_pointer_type p = malloc(sizeof *p)`. – Iharob Al Asimi Mar 02 '17 at 13:44
  • this is because you write ` internalhdl p = (internaldata*) malloc (sizeof(internaldata));` when p shoud be `internaldata`, not `internalhdl`. – Roy Avidan Mar 02 '17 at 13:45
  • See how you did `p.e` instead of `p->e`, and it's of course natural to do so. – Iharob Al Asimi Mar 02 '17 at 13:45
  • 1
    I'd go even further than Olaf's excellent suggestion. Never typedef away pointer semantics at all. Even a function pointer typedef should instead be function typedefs, with variables declared as pointers to those. – StoryTeller - Unslander Monica Mar 02 '17 at 13:46

2 Answers2

0

The most important thing you need to consider in my opinion, is that you will gain absolutely nothing but darkenss in doing something like this, and that you want to typedef a pointer to another type of pointer. If it is an opaque poitner, it doesn't make sense to typedef it also because you will never access the members of the underlying structure, it might very well be passed as a void * pointer, but when you allocate it, you MUST know it's type because the compiler needs to know it's size and layout in order to allocate it correctly (aligning it correctly for example would be impossible otherwise).

If you don't want to repeatedly use the sizeof operator to allocate the correct size there are two possible approaches1

  1. Use a macro

    #define allocate(x) x = malloc(sizeof(*x))
    

    and then

    my_type *x;
    allocate(x);
    

    but this is horrible and unclear.

  2. Use an allocation function,

    my_type *
    my_type_alloc()
    {
        return malloc(sizeof(my_type));
    }
    

    such that

    my_type *x;
    x = my_type_allocate();
    

    this is clean and simple, and you can't do it wrong.

Note that returning the appropriate pointer type just ensures that you will not accidentally do something that might cause undefined behavior, but allocation functions can simply return void * and they will work, that is why I did not cast malloc()'s return value.

Syntax sugar is something that you must be very careful with, because somtimes it looks like you simplified and improved syntax when what you did was hide vital information from the fellow programmer that will read your code, that programmer could even be yourself some time after writing the code.

And your workaround is actually causing one unecessary allocation. In fact, when you finally understand poitners, you will then really avoid unnecessary allocations by not copying data when you will only read from it.


1In both cases, you should check for NULL after the allocation is performed to ensure you can access such pointer without causing undefined behavior

Iharob Al Asimi
  • 52,653
  • 6
  • 59
  • 97
0

It would seem you are on the right track with the design, it is just the implementation that's questionable. As mentioned in comments, you should never hide pointers behind typedefs and opaque pointers is no exception to this. If the caller believes that these are allocated variables, they may decide to do stupid things like this:

set_obj(obj1, "foo"); // set private data to something
memcpy(&obj2, &obj1); // take a hardcopy of the data (or so we thought)
set_obj(obj1, "bar"); // set private data to something else
print_obj(obj2);      // prints "bar", wtf!!!

So stop hiding the pointers. With some slight modifications you should get the code to work as expected:

external.h

typedef struct external_t external_t;

external_t* external_create (/* parameters here */);

external.c

#include "external.h"

external_t* external_create (/* parameters here */)
{
  external_t* ext = malloc(sizeof *ext);
  /* initialize stuff here */
  return ext;
}

internal.h

#include "external.h"

typedef struct internal_t internal_t;

internal_t* internal_create (/* parameters here */);

internal.c

#include "internal.h"

struct internal_t
{
  external_t* ext;
};

internal_t* internal_create (/* parameters here */)
{
  internal_t* inter = malloc(sizeof *inter);
  inter->ext = external_create (/* parameters here */);
  if(inter->ext == NULL)
  {
    return NULL;
  }
  /* initialize stuff here */
  return inter;
}

The caller will have to use pointers too.


Also, there is no need to cast the result of malloc. Beat the dead horse here:
Do I cast the result of malloc?.

Community
  • 1
  • 1
Lundin
  • 195,001
  • 40
  • 254
  • 396