Polymorphism with overrides and base calls simulating an employee

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
5
down vote

favorite
2












I've studied and worked a lot on the subject for a month or two to get closer to the concept in a cleaner way, i.e. no virtual tables and no bunch of functions with prefixes etc. and most importantly functions are to be defined within structs as pointers to simulate OOP classes as much as possible.



Only constructors with prefixes are to be available publically. I also wanted to have the ability to call the override super method.



I'm not sure if following example is a compile time or run time polymorphism but it's satisfying the runtime aspects of it but then isn't it what we need without the need to have a look at the type of object? Please enlighten.



I'm putting it out there for critical analysis and review or problem with the approach.



The idea is to introduce a pointer of the parent type that retains a reference to original methods, the sub structures can override the parent implementation of the methods but the call to original methods are available via base pointer.



main.c



int main() 
Person* person = Person_new("John", "Doe");
Employee* employee = Employee_new("Jane", "Doe", "Acme", 40000);
person->display(person); // displaying Person object
puts("------");
employee->super.display(&employee->super); // displaying employee info
// ((Person *)employee)->display(employee); // or this
person->release(person);
employee->super.release(&employee->super);
return 0;



person.h



typedef struct Person Person;
struct Person
Person *base; // to keep access to original funcs in case of overrides
char* first;
char* last;
void (*display)(const Person *self);
void (*release)(Person *self);
;
Person* Person_new(char* first, char* last);


person.c



static void display(const Person *self) 
printf("First: %sn", self->first);
printf("Last: %sn", self->last);


static void release(Person *self)
free(self);


Person* Person_new(char* first, char* last)
Person* self = malloc(sizeof *self);
if(self == NULL) return NULL;

self->first = strdup(first);
self->last = strdup(last);
self->display = display;
self->release = release;

self->base = self; // assigning reference to self to retain original functions in case they get overridden

return self;



employee.h



typedef struct Employee Employee;
struct Employee
Person super;
char* company;
int salary;
;
Employee* Employee_new(char* first, char* last, char* company, int salary);


employee.c



static void display(const Person* self) 
self->base->display(self->base); // calling super class func

printf("Company: %sn", ((Employee *)self)->company);
printf("Salary: %dn", ((Employee*)self)->salary);


static void release(Person *self)
self->base->release(self->base); // calling super class func
free(self);


Employee* Employee_new(char* first, char* last, char* company, int salary)
Employee* employee = malloc(sizeof(Employee));
if(employee == NULL) return NULL;

employee->super = *Person_new(first, last);
employee->super.display = display; // override
employee->super.release = release; // override

employee->company = strdup(company);
employee->salary = salary;

return employee;




First: John
Last: Doe
---
First: Jane
Last: Doe
Company: Acme
Salary: 40000



Edit



Discussion with Pablo confirmed that the approach is correct. I'm still seeking some discussion on whether this approach falls under compile time or run time polymorphism.







share|improve this question





















  • This malloc(sizeof(char *) * (strlen(first) + 1)); is incorrect, it should be sizeof(char) which is defined to be 1, so you can just do malloc(strlen(first) + 1). And check the return value of malloc!
    – Pablo
    Mar 5 at 1:06










  • fixed, thanks Pablo
    – App2015
    Mar 5 at 1:12










  • Even 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.
    – chux
    Mar 5 at 1:15










  • Your second edit with the free is wrong, see my answer, I've updated it addressing that. You cannot free something and access it afterwards, and &employee->super does not return the address of the original pointer.
    – Pablo
    Mar 5 at 2:06







  • 3




    I have rolled back your last edit. Please don't change or add to the code in your question after you have received answers. See What should I do when someone answers my question? Thank you.
    – Phrancis
    Mar 5 at 2:59
















up vote
5
down vote

favorite
2












I've studied and worked a lot on the subject for a month or two to get closer to the concept in a cleaner way, i.e. no virtual tables and no bunch of functions with prefixes etc. and most importantly functions are to be defined within structs as pointers to simulate OOP classes as much as possible.



Only constructors with prefixes are to be available publically. I also wanted to have the ability to call the override super method.



I'm not sure if following example is a compile time or run time polymorphism but it's satisfying the runtime aspects of it but then isn't it what we need without the need to have a look at the type of object? Please enlighten.



I'm putting it out there for critical analysis and review or problem with the approach.



The idea is to introduce a pointer of the parent type that retains a reference to original methods, the sub structures can override the parent implementation of the methods but the call to original methods are available via base pointer.



main.c



int main() 
Person* person = Person_new("John", "Doe");
Employee* employee = Employee_new("Jane", "Doe", "Acme", 40000);
person->display(person); // displaying Person object
puts("------");
employee->super.display(&employee->super); // displaying employee info
// ((Person *)employee)->display(employee); // or this
person->release(person);
employee->super.release(&employee->super);
return 0;



person.h



typedef struct Person Person;
struct Person
Person *base; // to keep access to original funcs in case of overrides
char* first;
char* last;
void (*display)(const Person *self);
void (*release)(Person *self);
;
Person* Person_new(char* first, char* last);


person.c



static void display(const Person *self) 
printf("First: %sn", self->first);
printf("Last: %sn", self->last);


static void release(Person *self)
free(self);


Person* Person_new(char* first, char* last)
Person* self = malloc(sizeof *self);
if(self == NULL) return NULL;

self->first = strdup(first);
self->last = strdup(last);
self->display = display;
self->release = release;

self->base = self; // assigning reference to self to retain original functions in case they get overridden

return self;



employee.h



typedef struct Employee Employee;
struct Employee
Person super;
char* company;
int salary;
;
Employee* Employee_new(char* first, char* last, char* company, int salary);


employee.c



static void display(const Person* self) 
self->base->display(self->base); // calling super class func

printf("Company: %sn", ((Employee *)self)->company);
printf("Salary: %dn", ((Employee*)self)->salary);


static void release(Person *self)
self->base->release(self->base); // calling super class func
free(self);


Employee* Employee_new(char* first, char* last, char* company, int salary)
Employee* employee = malloc(sizeof(Employee));
if(employee == NULL) return NULL;

employee->super = *Person_new(first, last);
employee->super.display = display; // override
employee->super.release = release; // override

employee->company = strdup(company);
employee->salary = salary;

return employee;




First: John
Last: Doe
---
First: Jane
Last: Doe
Company: Acme
Salary: 40000



Edit



Discussion with Pablo confirmed that the approach is correct. I'm still seeking some discussion on whether this approach falls under compile time or run time polymorphism.







share|improve this question





















  • This malloc(sizeof(char *) * (strlen(first) + 1)); is incorrect, it should be sizeof(char) which is defined to be 1, so you can just do malloc(strlen(first) + 1). And check the return value of malloc!
    – Pablo
    Mar 5 at 1:06










  • fixed, thanks Pablo
    – App2015
    Mar 5 at 1:12










  • Even 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.
    – chux
    Mar 5 at 1:15










  • Your second edit with the free is wrong, see my answer, I've updated it addressing that. You cannot free something and access it afterwards, and &employee->super does not return the address of the original pointer.
    – Pablo
    Mar 5 at 2:06







  • 3




    I have rolled back your last edit. Please don't change or add to the code in your question after you have received answers. See What should I do when someone answers my question? Thank you.
    – Phrancis
    Mar 5 at 2:59












up vote
5
down vote

favorite
2









up vote
5
down vote

favorite
2






2





I've studied and worked a lot on the subject for a month or two to get closer to the concept in a cleaner way, i.e. no virtual tables and no bunch of functions with prefixes etc. and most importantly functions are to be defined within structs as pointers to simulate OOP classes as much as possible.



Only constructors with prefixes are to be available publically. I also wanted to have the ability to call the override super method.



I'm not sure if following example is a compile time or run time polymorphism but it's satisfying the runtime aspects of it but then isn't it what we need without the need to have a look at the type of object? Please enlighten.



I'm putting it out there for critical analysis and review or problem with the approach.



The idea is to introduce a pointer of the parent type that retains a reference to original methods, the sub structures can override the parent implementation of the methods but the call to original methods are available via base pointer.



main.c



int main() 
Person* person = Person_new("John", "Doe");
Employee* employee = Employee_new("Jane", "Doe", "Acme", 40000);
person->display(person); // displaying Person object
puts("------");
employee->super.display(&employee->super); // displaying employee info
// ((Person *)employee)->display(employee); // or this
person->release(person);
employee->super.release(&employee->super);
return 0;



person.h



typedef struct Person Person;
struct Person
Person *base; // to keep access to original funcs in case of overrides
char* first;
char* last;
void (*display)(const Person *self);
void (*release)(Person *self);
;
Person* Person_new(char* first, char* last);


person.c



static void display(const Person *self) 
printf("First: %sn", self->first);
printf("Last: %sn", self->last);


static void release(Person *self)
free(self);


Person* Person_new(char* first, char* last)
Person* self = malloc(sizeof *self);
if(self == NULL) return NULL;

self->first = strdup(first);
self->last = strdup(last);
self->display = display;
self->release = release;

self->base = self; // assigning reference to self to retain original functions in case they get overridden

return self;



employee.h



typedef struct Employee Employee;
struct Employee
Person super;
char* company;
int salary;
;
Employee* Employee_new(char* first, char* last, char* company, int salary);


employee.c



static void display(const Person* self) 
self->base->display(self->base); // calling super class func

printf("Company: %sn", ((Employee *)self)->company);
printf("Salary: %dn", ((Employee*)self)->salary);


static void release(Person *self)
self->base->release(self->base); // calling super class func
free(self);


Employee* Employee_new(char* first, char* last, char* company, int salary)
Employee* employee = malloc(sizeof(Employee));
if(employee == NULL) return NULL;

employee->super = *Person_new(first, last);
employee->super.display = display; // override
employee->super.release = release; // override

employee->company = strdup(company);
employee->salary = salary;

return employee;




First: John
Last: Doe
---
First: Jane
Last: Doe
Company: Acme
Salary: 40000



Edit



Discussion with Pablo confirmed that the approach is correct. I'm still seeking some discussion on whether this approach falls under compile time or run time polymorphism.







share|improve this question













I've studied and worked a lot on the subject for a month or two to get closer to the concept in a cleaner way, i.e. no virtual tables and no bunch of functions with prefixes etc. and most importantly functions are to be defined within structs as pointers to simulate OOP classes as much as possible.



Only constructors with prefixes are to be available publically. I also wanted to have the ability to call the override super method.



I'm not sure if following example is a compile time or run time polymorphism but it's satisfying the runtime aspects of it but then isn't it what we need without the need to have a look at the type of object? Please enlighten.



I'm putting it out there for critical analysis and review or problem with the approach.



The idea is to introduce a pointer of the parent type that retains a reference to original methods, the sub structures can override the parent implementation of the methods but the call to original methods are available via base pointer.



main.c



int main() 
Person* person = Person_new("John", "Doe");
Employee* employee = Employee_new("Jane", "Doe", "Acme", 40000);
person->display(person); // displaying Person object
puts("------");
employee->super.display(&employee->super); // displaying employee info
// ((Person *)employee)->display(employee); // or this
person->release(person);
employee->super.release(&employee->super);
return 0;



person.h



typedef struct Person Person;
struct Person
Person *base; // to keep access to original funcs in case of overrides
char* first;
char* last;
void (*display)(const Person *self);
void (*release)(Person *self);
;
Person* Person_new(char* first, char* last);


person.c



static void display(const Person *self) 
printf("First: %sn", self->first);
printf("Last: %sn", self->last);


static void release(Person *self)
free(self);


Person* Person_new(char* first, char* last)
Person* self = malloc(sizeof *self);
if(self == NULL) return NULL;

self->first = strdup(first);
self->last = strdup(last);
self->display = display;
self->release = release;

self->base = self; // assigning reference to self to retain original functions in case they get overridden

return self;



employee.h



typedef struct Employee Employee;
struct Employee
Person super;
char* company;
int salary;
;
Employee* Employee_new(char* first, char* last, char* company, int salary);


employee.c



static void display(const Person* self) 
self->base->display(self->base); // calling super class func

printf("Company: %sn", ((Employee *)self)->company);
printf("Salary: %dn", ((Employee*)self)->salary);


static void release(Person *self)
self->base->release(self->base); // calling super class func
free(self);


Employee* Employee_new(char* first, char* last, char* company, int salary)
Employee* employee = malloc(sizeof(Employee));
if(employee == NULL) return NULL;

employee->super = *Person_new(first, last);
employee->super.display = display; // override
employee->super.release = release; // override

employee->company = strdup(company);
employee->salary = salary;

return employee;




First: John
Last: Doe
---
First: Jane
Last: Doe
Company: Acme
Salary: 40000



Edit



Discussion with Pablo confirmed that the approach is correct. I'm still seeking some discussion on whether this approach falls under compile time or run time polymorphism.









share|improve this question












share|improve this question




share|improve this question








edited Mar 5 at 7:21
























asked Mar 5 at 0:15









App2015

1515




1515











  • This malloc(sizeof(char *) * (strlen(first) + 1)); is incorrect, it should be sizeof(char) which is defined to be 1, so you can just do malloc(strlen(first) + 1). And check the return value of malloc!
    – Pablo
    Mar 5 at 1:06










  • fixed, thanks Pablo
    – App2015
    Mar 5 at 1:12










  • Even 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.
    – chux
    Mar 5 at 1:15










  • Your second edit with the free is wrong, see my answer, I've updated it addressing that. You cannot free something and access it afterwards, and &employee->super does not return the address of the original pointer.
    – Pablo
    Mar 5 at 2:06







  • 3




    I have rolled back your last edit. Please don't change or add to the code in your question after you have received answers. See What should I do when someone answers my question? Thank you.
    – Phrancis
    Mar 5 at 2:59
















  • This malloc(sizeof(char *) * (strlen(first) + 1)); is incorrect, it should be sizeof(char) which is defined to be 1, so you can just do malloc(strlen(first) + 1). And check the return value of malloc!
    – Pablo
    Mar 5 at 1:06










  • fixed, thanks Pablo
    – App2015
    Mar 5 at 1:12










  • Even 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.
    – chux
    Mar 5 at 1:15










  • Your second edit with the free is wrong, see my answer, I've updated it addressing that. You cannot free something and access it afterwards, and &employee->super does not return the address of the original pointer.
    – Pablo
    Mar 5 at 2:06







  • 3




    I have rolled back your last edit. Please don't change or add to the code in your question after you have received answers. See What should I do when someone answers my question? Thank you.
    – Phrancis
    Mar 5 at 2:59















This malloc(sizeof(char *) * (strlen(first) + 1)); is incorrect, it should be sizeof(char) which is defined to be 1, so you can just do malloc(strlen(first) + 1). And check the return value of malloc!
– Pablo
Mar 5 at 1:06




This malloc(sizeof(char *) * (strlen(first) + 1)); is incorrect, it should be sizeof(char) which is defined to be 1, so you can just do malloc(strlen(first) + 1). And check the return value of malloc!
– Pablo
Mar 5 at 1:06












fixed, thanks Pablo
– App2015
Mar 5 at 1:12




fixed, thanks Pablo
– App2015
Mar 5 at 1:12












Even 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.
– chux
Mar 5 at 1:15




Even 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.
– chux
Mar 5 at 1:15












Your second edit with the free is wrong, see my answer, I've updated it addressing that. You cannot free something and access it afterwards, and &employee->super does not return the address of the original pointer.
– Pablo
Mar 5 at 2:06





Your second edit with the free is wrong, see my answer, I've updated it addressing that. You cannot free something and access it afterwards, and &employee->super does not return the address of the original pointer.
– Pablo
Mar 5 at 2:06





3




3




I have rolled back your last edit. Please don't change or add to the code in your question after you have received answers. See What should I do when someone answers my question? Thank you.
– Phrancis
Mar 5 at 2:59




I have rolled back your last edit. Please don't change or add to the code in your question after you have received answers. See What should I do when someone answers my question? Thank you.
– Phrancis
Mar 5 at 2:59










2 Answers
2






active

oldest

votes

















up vote
4
down vote



accepted










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))
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) 


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: %sn", self->first);
printf("Last: %sn", 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: %sn", e->company);
printf("Salary: %dn", 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.






share|improve this answer



















  • 1




    Let us continue this discussion in chat.
    – App2015
    Mar 5 at 1:19










  • I read your edit3, in other words are you saying that my original approach was correct? I'm confused a bit going in both directions now
    – App2015
    Mar 5 at 4:32










  • @App2015 yes, your approach was correct, I didn't realize it right away, I had to step through the code with the debugger.
    – Pablo
    Mar 5 at 4:34










  • Thanks Pablo. I liked the fact that you have valgrind, I couldn't have it on Mac. one last favor can you download this and review if everything is intact, I've spent a great deal on this to get it right, also would you like to discuss if this is runtime or compile time polymorphism? Download: dropbox.com/s/q4vdpicj1aq17ao/Polymorphism.zip?dl=0
    – App2015
    Mar 5 at 4:43










  • "There is one thing I don't line about" You probably mean like about?
    – yuri
    Mar 5 at 8:54

















up vote
2
down vote













Question-able shallow copy of names



Below code makes a copy of a pointer, that given OP's example, is a pointer to a string literal - in which case, self->first = first; self->last = last; both should have been const char *. I would expect an allocation of memory to duplicate the strings with strdup() or its equivalent function - sample. strdup() is not part of the standard C library, yet commonly available.



// self->first = first;
// self->last = last;
self->first = strdup(first);
self->last = strdup(last);





share|improve this answer























  • sure, I'm going to edit it based on your suggestions
    – App2015
    Mar 5 at 0:51






  • 1




    @App2015 please read the help section on updating your question. It's generally not allowed.
    – user1118321
    Mar 5 at 0:53










  • sorry about that.
    – App2015
    Mar 5 at 1:04










  • @App2015 The update 1) should not have been done per this site's standards 2) malloc(sizeof(char *) * (strlen(first) + 1)); is an excessive allocation
    – chux
    Mar 5 at 1:05











  • what makes it excessive?
    – App2015
    Mar 5 at 1:41










Your Answer




StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: false,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);



);








 

draft saved


draft discarded


















StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f188826%2fpolymorphism-with-overrides-and-base-calls-simulating-an-employee%23new-answer', 'question_page');

);

Post as a guest






























2 Answers
2






active

oldest

votes








2 Answers
2






active

oldest

votes









active

oldest

votes






active

oldest

votes








up vote
4
down vote



accepted










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))
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) 


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: %sn", self->first);
printf("Last: %sn", 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: %sn", e->company);
printf("Salary: %dn", 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.






share|improve this answer



















  • 1




    Let us continue this discussion in chat.
    – App2015
    Mar 5 at 1:19










  • I read your edit3, in other words are you saying that my original approach was correct? I'm confused a bit going in both directions now
    – App2015
    Mar 5 at 4:32










  • @App2015 yes, your approach was correct, I didn't realize it right away, I had to step through the code with the debugger.
    – Pablo
    Mar 5 at 4:34










  • Thanks Pablo. I liked the fact that you have valgrind, I couldn't have it on Mac. one last favor can you download this and review if everything is intact, I've spent a great deal on this to get it right, also would you like to discuss if this is runtime or compile time polymorphism? Download: dropbox.com/s/q4vdpicj1aq17ao/Polymorphism.zip?dl=0
    – App2015
    Mar 5 at 4:43










  • "There is one thing I don't line about" You probably mean like about?
    – yuri
    Mar 5 at 8:54














up vote
4
down vote



accepted










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))
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) 


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: %sn", self->first);
printf("Last: %sn", 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: %sn", e->company);
printf("Salary: %dn", 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.






share|improve this answer



















  • 1




    Let us continue this discussion in chat.
    – App2015
    Mar 5 at 1:19










  • I read your edit3, in other words are you saying that my original approach was correct? I'm confused a bit going in both directions now
    – App2015
    Mar 5 at 4:32










  • @App2015 yes, your approach was correct, I didn't realize it right away, I had to step through the code with the debugger.
    – Pablo
    Mar 5 at 4:34










  • Thanks Pablo. I liked the fact that you have valgrind, I couldn't have it on Mac. one last favor can you download this and review if everything is intact, I've spent a great deal on this to get it right, also would you like to discuss if this is runtime or compile time polymorphism? Download: dropbox.com/s/q4vdpicj1aq17ao/Polymorphism.zip?dl=0
    – App2015
    Mar 5 at 4:43










  • "There is one thing I don't line about" You probably mean like about?
    – yuri
    Mar 5 at 8:54












up vote
4
down vote



accepted







up vote
4
down vote



accepted






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))
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) 


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: %sn", self->first);
printf("Last: %sn", 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: %sn", e->company);
printf("Salary: %dn", 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.






share|improve this answer















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))
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) 


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: %sn", self->first);
printf("Last: %sn", 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: %sn", e->company);
printf("Salary: %dn", 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.







share|improve this answer















share|improve this answer



share|improve this answer








edited Mar 5 at 2:58


























answered Mar 5 at 0:37









Pablo

2018




2018







  • 1




    Let us continue this discussion in chat.
    – App2015
    Mar 5 at 1:19










  • I read your edit3, in other words are you saying that my original approach was correct? I'm confused a bit going in both directions now
    – App2015
    Mar 5 at 4:32










  • @App2015 yes, your approach was correct, I didn't realize it right away, I had to step through the code with the debugger.
    – Pablo
    Mar 5 at 4:34










  • Thanks Pablo. I liked the fact that you have valgrind, I couldn't have it on Mac. one last favor can you download this and review if everything is intact, I've spent a great deal on this to get it right, also would you like to discuss if this is runtime or compile time polymorphism? Download: dropbox.com/s/q4vdpicj1aq17ao/Polymorphism.zip?dl=0
    – App2015
    Mar 5 at 4:43










  • "There is one thing I don't line about" You probably mean like about?
    – yuri
    Mar 5 at 8:54












  • 1




    Let us continue this discussion in chat.
    – App2015
    Mar 5 at 1:19










  • I read your edit3, in other words are you saying that my original approach was correct? I'm confused a bit going in both directions now
    – App2015
    Mar 5 at 4:32










  • @App2015 yes, your approach was correct, I didn't realize it right away, I had to step through the code with the debugger.
    – Pablo
    Mar 5 at 4:34










  • Thanks Pablo. I liked the fact that you have valgrind, I couldn't have it on Mac. one last favor can you download this and review if everything is intact, I've spent a great deal on this to get it right, also would you like to discuss if this is runtime or compile time polymorphism? Download: dropbox.com/s/q4vdpicj1aq17ao/Polymorphism.zip?dl=0
    – App2015
    Mar 5 at 4:43










  • "There is one thing I don't line about" You probably mean like about?
    – yuri
    Mar 5 at 8:54







1




1




Let us continue this discussion in chat.
– App2015
Mar 5 at 1:19




Let us continue this discussion in chat.
– App2015
Mar 5 at 1:19












I read your edit3, in other words are you saying that my original approach was correct? I'm confused a bit going in both directions now
– App2015
Mar 5 at 4:32




I read your edit3, in other words are you saying that my original approach was correct? I'm confused a bit going in both directions now
– App2015
Mar 5 at 4:32












@App2015 yes, your approach was correct, I didn't realize it right away, I had to step through the code with the debugger.
– Pablo
Mar 5 at 4:34




@App2015 yes, your approach was correct, I didn't realize it right away, I had to step through the code with the debugger.
– Pablo
Mar 5 at 4:34












Thanks Pablo. I liked the fact that you have valgrind, I couldn't have it on Mac. one last favor can you download this and review if everything is intact, I've spent a great deal on this to get it right, also would you like to discuss if this is runtime or compile time polymorphism? Download: dropbox.com/s/q4vdpicj1aq17ao/Polymorphism.zip?dl=0
– App2015
Mar 5 at 4:43




Thanks Pablo. I liked the fact that you have valgrind, I couldn't have it on Mac. one last favor can you download this and review if everything is intact, I've spent a great deal on this to get it right, also would you like to discuss if this is runtime or compile time polymorphism? Download: dropbox.com/s/q4vdpicj1aq17ao/Polymorphism.zip?dl=0
– App2015
Mar 5 at 4:43












"There is one thing I don't line about" You probably mean like about?
– yuri
Mar 5 at 8:54




"There is one thing I don't line about" You probably mean like about?
– yuri
Mar 5 at 8:54












up vote
2
down vote













Question-able shallow copy of names



Below code makes a copy of a pointer, that given OP's example, is a pointer to a string literal - in which case, self->first = first; self->last = last; both should have been const char *. I would expect an allocation of memory to duplicate the strings with strdup() or its equivalent function - sample. strdup() is not part of the standard C library, yet commonly available.



// self->first = first;
// self->last = last;
self->first = strdup(first);
self->last = strdup(last);





share|improve this answer























  • sure, I'm going to edit it based on your suggestions
    – App2015
    Mar 5 at 0:51






  • 1




    @App2015 please read the help section on updating your question. It's generally not allowed.
    – user1118321
    Mar 5 at 0:53










  • sorry about that.
    – App2015
    Mar 5 at 1:04










  • @App2015 The update 1) should not have been done per this site's standards 2) malloc(sizeof(char *) * (strlen(first) + 1)); is an excessive allocation
    – chux
    Mar 5 at 1:05











  • what makes it excessive?
    – App2015
    Mar 5 at 1:41














up vote
2
down vote













Question-able shallow copy of names



Below code makes a copy of a pointer, that given OP's example, is a pointer to a string literal - in which case, self->first = first; self->last = last; both should have been const char *. I would expect an allocation of memory to duplicate the strings with strdup() or its equivalent function - sample. strdup() is not part of the standard C library, yet commonly available.



// self->first = first;
// self->last = last;
self->first = strdup(first);
self->last = strdup(last);





share|improve this answer























  • sure, I'm going to edit it based on your suggestions
    – App2015
    Mar 5 at 0:51






  • 1




    @App2015 please read the help section on updating your question. It's generally not allowed.
    – user1118321
    Mar 5 at 0:53










  • sorry about that.
    – App2015
    Mar 5 at 1:04










  • @App2015 The update 1) should not have been done per this site's standards 2) malloc(sizeof(char *) * (strlen(first) + 1)); is an excessive allocation
    – chux
    Mar 5 at 1:05











  • what makes it excessive?
    – App2015
    Mar 5 at 1:41












up vote
2
down vote










up vote
2
down vote









Question-able shallow copy of names



Below code makes a copy of a pointer, that given OP's example, is a pointer to a string literal - in which case, self->first = first; self->last = last; both should have been const char *. I would expect an allocation of memory to duplicate the strings with strdup() or its equivalent function - sample. strdup() is not part of the standard C library, yet commonly available.



// self->first = first;
// self->last = last;
self->first = strdup(first);
self->last = strdup(last);





share|improve this answer















Question-able shallow copy of names



Below code makes a copy of a pointer, that given OP's example, is a pointer to a string literal - in which case, self->first = first; self->last = last; both should have been const char *. I would expect an allocation of memory to duplicate the strings with strdup() or its equivalent function - sample. strdup() is not part of the standard C library, yet commonly available.



// self->first = first;
// self->last = last;
self->first = strdup(first);
self->last = strdup(last);






share|improve this answer















share|improve this answer



share|improve this answer








edited Mar 5 at 1:02


























answered Mar 5 at 0:49









chux

11.4k11238




11.4k11238











  • sure, I'm going to edit it based on your suggestions
    – App2015
    Mar 5 at 0:51






  • 1




    @App2015 please read the help section on updating your question. It's generally not allowed.
    – user1118321
    Mar 5 at 0:53










  • sorry about that.
    – App2015
    Mar 5 at 1:04










  • @App2015 The update 1) should not have been done per this site's standards 2) malloc(sizeof(char *) * (strlen(first) + 1)); is an excessive allocation
    – chux
    Mar 5 at 1:05











  • what makes it excessive?
    – App2015
    Mar 5 at 1:41
















  • sure, I'm going to edit it based on your suggestions
    – App2015
    Mar 5 at 0:51






  • 1




    @App2015 please read the help section on updating your question. It's generally not allowed.
    – user1118321
    Mar 5 at 0:53










  • sorry about that.
    – App2015
    Mar 5 at 1:04










  • @App2015 The update 1) should not have been done per this site's standards 2) malloc(sizeof(char *) * (strlen(first) + 1)); is an excessive allocation
    – chux
    Mar 5 at 1:05











  • what makes it excessive?
    – App2015
    Mar 5 at 1:41















sure, I'm going to edit it based on your suggestions
– App2015
Mar 5 at 0:51




sure, I'm going to edit it based on your suggestions
– App2015
Mar 5 at 0:51




1




1




@App2015 please read the help section on updating your question. It's generally not allowed.
– user1118321
Mar 5 at 0:53




@App2015 please read the help section on updating your question. It's generally not allowed.
– user1118321
Mar 5 at 0:53












sorry about that.
– App2015
Mar 5 at 1:04




sorry about that.
– App2015
Mar 5 at 1:04












@App2015 The update 1) should not have been done per this site's standards 2) malloc(sizeof(char *) * (strlen(first) + 1)); is an excessive allocation
– chux
Mar 5 at 1:05





@App2015 The update 1) should not have been done per this site's standards 2) malloc(sizeof(char *) * (strlen(first) + 1)); is an excessive allocation
– chux
Mar 5 at 1:05













what makes it excessive?
– App2015
Mar 5 at 1:41




what makes it excessive?
– App2015
Mar 5 at 1:41












 

draft saved


draft discarded


























 


draft saved


draft discarded














StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f188826%2fpolymorphism-with-overrides-and-base-calls-simulating-an-employee%23new-answer', 'question_page');

);

Post as a guest













































































Popular posts from this blog

Python Lists

Aion

JavaScript Array Iteration Methods