alelievr / libft-unit-test

455 stars 88 forks source link

ft_lstdelone() test should be updated !! #71

Closed ombhd closed 4 years ago

ombhd commented 4 years ago

The tester is working on the old ft_lstdelone() function which has the prototype [ void ft_lstdelone(t_list **alst, void (*del)(void*, size_t)); ] , while the new subject has a function ft_lstdelone() which has the prototype [void ft_lstdelone(t_list *lst, void (*del)(void*));] and also the description is changed ! the old function was deleting the content and the node, while the new one deletes just the content ! Please change the test to test just the node's content if it is NULL or not ! Thanks in advance !

abeaugustijn commented 4 years ago

I submitted a pull request to fix this issue

CyberDuck79 commented 4 years ago

Your correction is not OK, at least in the french subject it is clear : apply the pointed function on the content (most of the time it would be free()) and free the element (t_list)'. So the pointed function (wich free the content) is OK no need to change it, it's just that it check if the pointer is NULL after the function but you can't set it to NULL because the first argument is a simple pointer and not a pointer on pointer like in the old subject. I don't know how to do it but a proper check would be to check if the two memory spaces have been released. (no memory leaks)

ggjulio commented 4 years ago

@CyberDuck79 Yes, it's why I haven't figured out a way to fix that test...

s-t-a-n commented 4 years ago

SANDBOX_IRAISE( free(l); ); Could be a start?

alelievr commented 4 years ago

Hey, i made a quick fix here: https://github.com/alelievr/libft-unit-test/pull/75 but i don't have the new subject and tests (i'm not at school anymore). Can you verify that the tests are compliant with the subject ?

ggjulio commented 4 years ago

@alelievr There is a way with you macro to catch a double free and return success ? ft_lstdelone(l, lstdelone_f); free(l->content);

if there is no double free, test fail. And if there is, test ok.

alelievr commented 4 years ago

Fixed merged but without the double free test, will need to add it in the future c2f60f369fbd5f1125e0479fdfa2d389b5279fc0