1

i'm making a memory allocation/deallocation function
it's simple 

inline void safedealloc ( void *mem )
{
     if ( mem ) { free( mem ); mem = NULL; }
}

it's working fine .. however after using it few times with a program
i noticed that when calling it in something like 

safedealloc( (char *)name );
safedealloc( (char *)name );

causes error .. name supposed to be null after first call . but it doesn't and the second call with invalid pointer ofc causes error .. why isn't null being assigned to name as it should ?

PS: name is properly allocated using malloc and with valid size and contents

VirusEcks
  • 596
  • 1
  • 10
  • 14
  • Why do you test that `mem` is not null before passing it to `free`? – James McNellis Jun 11 '11 at 17:44
  • 2
    If you think this sort of function is necessary, I'd recommend getting [a good introductory C++ book](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) and learning to program in C++ correctly. You should [use automatic memory management in C++](http://stackoverflow.com/questions/3681455/what-is-the-philosophy-of-managing-memory-in-c), which means you should rarely be using `free` at all (and if you do, it should be very rare and in library code). – James McNellis Jun 11 '11 at 17:52

4 Answers4

7

It almost certainly is not working fine, it is pointless, and in C++ the way you allocate memory is with new and delete, not malloc and free. There is no need to check for a NULL pointer - free (NULL ) is well defined in both C and C++. And your assignment of NULL to the function parameter does nothing - it only modifies the parameter, not its value back in the calling program.

And lastly, if you find yourself writing code like this:

char * p = malloc(100);
safefree( p );
safefree( p );

the thing to do is to fix the code so you don't free the pointer twice, not to obfuscate things by sticking a band-aid on bad code.

  • 1
    Setting it to NULL isn't just to prevent double free, it also gives you an easy way to check whether it is has already been free'd so that you don't end up using a bad pointer. – Peter Alexander Jun 11 '11 at 17:58
  • +1 Because you're so very right in this answer, and about the other answer. – Omnifarious Jun 11 '11 at 17:59
  • 1
    @Peter Alexander: Use a smart pointer class if you want that kind of functionality. Don't hack together some `safefree` abortion that uses `free` when it should be using `delete`. – Omnifarious Jun 11 '11 at 18:01
  • @Omnifarious: He may not want the overhead and code bloat of smart pointers. – Peter Alexander Jun 11 '11 at 18:04
  • 3
    @peter Filling your code full of `if ( p == NULL )` stuff is horrible programming practice. –  Jun 11 '11 at 18:04
  • 1
    @Peter Alexander: It's less code bloat and overhead than calling a special function every time you want to delete something. Especially since in order to work reasonably that function has to be a template function anyway. `::std::auto_ptr` or `::std::unique_ptr` are practically 0 overhead, would offer this functionality and be relatively clean. – Omnifarious Jun 11 '11 at 18:05
  • @Peter What "overhead"? He's already got an overhead! And what "code bloat"? –  Jun 11 '11 at 18:06
  • 1
    @Peter: Even if smart pointers did add "overhead and code bloat," bloated, correct code is far superior to slimmer, incorrect code. – James McNellis Jun 11 '11 at 18:09
  • @Neil @Omnifarious: When using memory pools smart pointers require an extra pointer to the pool to de-allocate from. This means that pointers are double the size, bloating all code that copies the pointer. Also, shared pointers introduce bloat by placing inlined destructor code/calls everywhere where the ref count could reach 0. – Peter Alexander Jun 11 '11 at 18:12
  • @James: There's nothing incorrect about the code. Also, who are you to judge how important slim code is to the OP? Slim code is very important on embedded platforms. – Peter Alexander Jun 11 '11 at 18:14
  • 1
    @Peter Say what? where did "memory pools" suddenly appear from? –  Jun 11 '11 at 18:14
  • @Peter The OP never mentioned "slim code" or "embedded platforms", you introduced these red herrings. –  Jun 11 '11 at 18:15
  • @Neil: Exactly. He never mentioned them, and never mentioned a lack of them. So why pass judgement on design choices that you don't know the rationale behind? – Peter Alexander Jun 11 '11 at 18:19
  • 4
    @Peter There is obviously no "memory pool", because he is using free(). And if it was an embedded platform, he would probably be very resistant to using dynamic allocation at all. I call bullsh*t. –  Jun 11 '11 at 18:28
5

mem = NULL; doesn't do anything, because mem is a different variable.

Try this instead (notice the & in front of mem, which means a reference to mem):

template<typename T> //I added this due to request, since this doesn't quite work
                     //with pointers other than void*
inline void safedealloc ( T *& mem )
{
     if ( mem ) { free( mem ); mem = NULL; }
}

Whether this is actually a good idea or not is a completely different question.
The answer is No. Why? Because if you made this mistake here, chances are you made the same mistake elsewhere, and the problem just starts all over again, except this time it's higher on the stack and harder to find.

user541686
  • 205,094
  • 128
  • 528
  • 886
0

You're trying to change the address inside mem, but this only works on the local argument.

You pass the pointer by value, meaning you can't change the value of the variable the caller has.

Gilad Naor
  • 20,752
  • 14
  • 46
  • 53
0

You are passing pointer by value, that's the problem. Pass pointer to a pointer, or reference to a pointer, then your assignment will work.

Dmitry
  • 6,590
  • 2
  • 26
  • 19