There is one thing I don't line about Employee
: you are leaking memory.
Person super;
should be a pointer, not the object. You are creating the
object with malloc
, by doing
employee->super = *Person_new(first, last);
you are leaking memory because you lose the reference of the pointer returned
by malloc
and you cannot free it when you destroy the object. And you are not
checking if malloc
is returning 0. So I'd change it to:
typedef struct Employee Employee;
struct Employee {
Person super;
char* company;
int salary;
Person *super_ref;
};
Person* Employee_new(char* first, char* last, char* company, int salary) {
Employee* employee = malloc(sizeof(Employee));
if(employee == NULL)
return NULL;
employee->super_ref = *Person_new(first, last);
if(employee->super_ref == NULL)
{
free(employee);
return NULL;
}
employee->super = *employee->super_ref;
employee->super.display = display; // override
employee->company = company;
employee->salary = salary;
return (Person *)employee;
}
so now the destroy method can do free(obj->super_ref);
to free the memory of
the base object.
edit
An alternative is that you create a init function for Person
objects that gets
an object as an argument and does not allocate memory by itself. I'd do:
int Person_init(Person *self, char* first, char* last))
{
if(self == NULL || first == NULL || last == NULL)
return NULL;
self->first = first;
self->last = last;
self->display = display;
self->base = self; // assigning reference to self to retain original functions in case they get overriden
return 1;
}
Person* Person_new(char* first, char* last) {
Person* self = malloc(sizeof *self);
if(self == NULL)
return NULL;
if(Person_init(self, first, last) == 0)
{
free(self);
return NULL;
}
return self;
}
and for the Employee
class
Person* Employee_new(char* first, char* last, char* company, int salary) {
if(first == NULL || last == NULL || company == NULL)
return NULL;
Employee* employee = malloc(sizeof *employee);
if(employee == NULL)
return NULL;
if(Person_init(&employee->super, first, last) == 0)
{
free(employee);
return NULL;
}
employee->super.display = display; // override
employee->company = company;
employee->salary = salary;
return (Person *)employee;
}
Then you don't have to worry about freeing the base object.
edit2
*OP said in the comments**
I'm wondering what's wrong with the freeing the base pointer, I'm assigning it right away to the new value free(&employee->super); employee->super = *super;
Sorry, it seem to me you don't have a real understanding of memory managment and
what the *
dereferencing operator does.
First of all, you have no guarantee that after free(ptr)
you can access
the contents of ptr
. After the free
, ptr
points to an invalid location and
accessing/dereferencing it is undefined behaviour.
Secondly
employee->super = *Person_new(first, last);
is the same as doing
Person *tmp = Person_new(first, last);
employee->super = *tmp;
*tmp
is dereferencing tmp
, the type is Person
not Person*
. In C when you
do an assigment with struct objects (not pointers) you are copying bit by bit
the bit pattern into a new object. That means that the bit pattern of
employee->super
is the same as the bit pattern of *tmp
, but they are not
the sam object, the reside at different places in memory.
You can only call free(ptr)
when ptr
stores the address returned by either
malloc
or realloc
. If you do free(&employee->super)
you are passing a
completely different address to that one that malloc
returned in Person_new
,
this is undefined behaviour, you cannot do that.
There is no other way for you, you have to store the original pointer in the
struct, that's the reason why I added the Person *super_ref
in the struct, so
that the original pointer is not lost.
We have a major problem, this implementation is causing recursion at
I made small changes to your code, I don't have this problem:
#include <stdlib.h>
#include <stdio.h>
#include <stddef.h>
typedef struct Person Person;
typedef struct Person {
Person *base; // reference to itself to retain reference to original function
char* first;
char* last;
void (*display)(const Person *self);
void (*destroy)(Person *self);
} Person;
Person* Person_new(char* first, char* last);
static void display(const Person *self) {
printf("First: %s\n", self->first);
printf("Last: %s\n", self->last);
}
static void destroy_person(Person *self)
{
if(self == NULL)
return;
free(self);
}
Person* Person_new(char* first, char* last) {
Person* self = malloc(sizeof(Person));
self->first = first;
self->last = last;
self->display = display;
self->destroy = destroy_person;
self->base = self; // assigning reference to self to retain original functions in case they get overriden
return self;
}
typedef struct Employee Employee;
struct Employee {
Person super;
char* company;
int salary;
};
Person* Employee_new(char* first, char* last, char* company, int salary);
static void display2(const Person* self) {
self->base->display(self->base);
Employee *e = (Employee*) self;
printf("Company: %s\n", e->company);
printf("Salary: %d\n", e->salary);
}
static void destroy_employee(Person *self)
{
if(self == NULL)
return;
if(self->base)
self->base->destroy(self->base);
free(self);
}
Person* Employee_new(char* first, char* last, char* company, int salary) {
Employee* employee = malloc(sizeof(Employee));
employee->super = *Person_new(first, last);
employee->super.display = display2; // override
employee->super.destroy = destroy_employee; // override
employee->company = company;
employee->salary = salary;
return (Person*) employee;
}
int main(void)
{
Person* person = Person_new("John", "Doe");
Person* employee = Employee_new("Jane", "Doe", "Acme", 40000);
person->display(person);
puts("---");
employee->display(employee);
person->destroy(person);
employee->destroy(employee);
}
It prints
$ ./b
First: John
Last: Doe
---
First: Jane
Last: Doe
Company: Acme
Salary: 40000
edit3
The fact that my little changes do not have the problem of endless recursion in
display2
kept me thinking about it and I've been running the code with the
debugger to see why I didn't have the problem as well. And now I understand why
my code does not have that problem, and your's shouldn't have that as well. In
fact, I realized that there is a way of freeing the memory correctly without
have a separate member like super_ref
.
Let me explain:
In Employee_new
you do:
employee->super = *Person_new(first, last);
employee->super.display = display2; // override
I told you that you would be leaking memory because you lose the original
pointer that malloc
returns. And that is true, however I failed to realize
that you have in fact a pointer pointing to the original address that malloc
returns: the base
member of the Person
struct. In Person_new
you do
self->base = self;
and this solves the problem with the overloading and the freeing, even though we
both didn't realize it at the time.
So again, when you do
employee->super = *Person_new(first, last);
you are dereferencing the pointer returned by malloc
and making a copy of the
object into another object. But I failed to realize is that
employee->super->base
points to the original location returned by malloc
.
So when you do
employee->super.display = display2; // override
you are indeed setting a new value to employee->super.display
, but because
employee->super
is just a copy, the original does not change and with
employee->super->base
you get the orginal object.
That's why
static void display2(const Person* self) {
self->base->display(self->base);
...
}
shouldn't do end in a recursion, because self->display
points to display2
,
but self->base->display
is still unchanged and points to display
.
And now you can use the same behaviour for destroying the Employee object.
Let's say you have destroy
function pointer in Person
which points to the
destroy function:
void destroy_person(Person *person)
{
if(person == NULL)
return NULL;
free(person);
}
and in Person_new
you add:
self->destroy = destroy_person;
Now you can destroy a person object by doing person->destroy(person);
.
The destroy
for Employee
will look similar to display2
:
void destroy_employee(Person *self)
{
if(self == NULL)
return;
if(self->base)
self->base->destroy(self->base);
free(self);
}
because self->base
is still pointing to the original object,
self->base->destroy
points to destroy_person
. Now in
Employee_new
you have to add
employee->super.display = display2; // override
employee->super.destroy = destroy_employee; // override
and in main
you just do person->destroy(person)
and
employee->destroy(employee);
. I've checked the code above (I've updated with
the destroy functions) with valgrind and it told me that everything was freed
correctly.
malloc(sizeof(char *) * (strlen(first) + 1));
is incorrect, it should besizeof(char)
which is defined to be 1, so you can just domalloc(strlen(first) + 1)
. And check the return value ofmalloc
! \$\endgroup\$malloc(sizeof(char) * (strlen(last) + 1));
is debatable to if that is the preferable code. Rolling post post to OP's original to avoid a moving review target. \$\endgroup\$&employee->super
does not return the address of the original pointer. \$\endgroup\$