tags:

views:

437

answers:

7

I have a global pointer variable

char* pointer = new char[500];
/* some operations... */

there is a seperate FreeGlobal() function that does free up the pointer as below:

delete[] pointer;

First time when the function is called, it actually frees up the memory and now the pointer is a bad pointer. But when we call this more than once, it throws an exception.

Is there a way to check the pointer variable before calling delete [] again? What are the work arounds? Is this a bad practice?

Thank you.

+7  A: 

Set pointer to null after you delete it. You should not try to delete the same data more than once.

As mentioned by GRB in the comments for this post, it is perfectly safe to call delete[] NULL.

David Andres
If you set the pointer to NULL, then the check before calling delete[] is unnecessary. The standard guarantees that deleting a null pointer is safe (and does nothing).
GRB
Duly noted: but it's a good check to make just in case. As mentioned in the OP, this is a global pointer and you can't be too sure of how it's used/misused elsewhere in code.
David Andres
With all due respect, it really isn't. Given that `delete NULL;` is a perfectly safe operation, what would you be checking for? The primary danger when using `delete` is that you delete a pointer that has already been deleted, but there's no way to check for that beforehand if the pointer hasn't been set to NULL (precisely the problem this question is trying to solve). So, given that there's no useful check that makes your program any safer, I don't really understand the point.
GRB
You're right. Post edited.
David Andres
`delete NULL;` is an invalid operation - just to be sure no-one gets that wrong. In any case, of course such an if would be totally useless. Better do `assert(ptr != 0);` if the invariant is that `ptr` cannot be 0, instead of an useless if - which will possibly let the bug go unnoticed.
Johannes Schaub - litb
litb is right of course. If your code is even deleting NULL, you know you're doing something wrong.
GRB
That is why globals are so wrong. Once again OOP! :P
Partial
@Partial Globals have their issues, but there is nothing in OOP that says you can't have globals. OOP is about encapsulation, inheritance, and polymorphism.
Dima
@Dima: What is encapsulation if not a way to remove globals? Have a look at my answer ;)
Partial
@Partial Encapsulation is creating an abstraction by packaging data and functionality into an object. You can easily have a perfectly encapsulated global object, if your program requires it. You should use globals judiciously, but there is nothing "anti-OOP" about them.
Dima
I think we should get clear on what we mean by "global". I mean things defined in a namespace outside all user defined namespaces. If i have free functions/objects, i prefer to put them into user defined namespaces, though. The term "global" is misused for things within user defined namespaces too, often. So we should clarify before discussing further.
Johannes Schaub - litb
A global variable is this:int variable;int main { //...}
Partial
Anyway, if your not going to use the advantages of C++ you should be doing C instead. It is lower level language than C++ and doing global variables would perhaps be justified that way... perhaps.
Partial
Thanks David and GRB. I think setting it to NULL is a work around. I understand that it is indeed bad to delete twice or delete a "bad" pointer. Thanks others and your suggestions to use memory safe mechanisms and C++ libs.
bugboy
@Partial class Foo{...}; Foo f; int main() {...} Is f not a global variable?
Dima
bugboy: Workaround or not, setting it to null is defensive programming. Usually you program will not destroy critical structures during a null-dereference. Freeing a buffer twice can corrupt your heap silently and your program can run merrily for hours afterward corrupting who know what before it eventually crashes.
jmucchiello
@Dima: Yes.. and why would you be using a global object?
Partial
`namespace F { int variable; }` here, it is not a global variable. Many people however say it *is* (www.cplusplus.com does, too, which is among the reason i *discourage* from recommending it).
Johannes Schaub - litb
@litb: What is a global variable? I would have said it's one at namespace scope. If a "global variable" is only one that's in the global namespace, then what is `variable` in your example?
sbi
It's a non-global variable which is declared into namespace scope, of course. The Standard of course clearly says "The outermost declarative region of a translation unit is also a namespace, called the global namespace. A name declared in the global namespace has global namespace scope (also called global scope). The potential scope of such a name begins at its point of declaration (3.3.1) and ends at the end of the translation unit that is its declarative region. Names with global namespace scope are said to be global." The variable may store "global state", but it's not global itself.
Johannes Schaub - litb
@litb: I was always reading "global variable" as a _concept_, rather than a syntactical construct. From a conceptual POV, whether someone wraps a namespace around a global variable doesn't make a difference.
sbi
+1  A: 

Your freeGlobal() function should make it point to 0, so define the function as such:

//if ptr is not null, delete; otherwise, return
void freeGlobal(char*& ptr)
{
   if (ptr != 0)
   {
      delete[] ptr;
      ptr = 0;
   }
}

Edit: 0 == NULL, at least for now, in C++.

Hooked
?? ptr is passed by value. Setting ptr=0 won't have any effect on the pointer that was passed to the function.
mobrule
That doesn't work. Try void freeGlobal(char* instead.
jmucchiello
You don't need the if. delete and delete[] check for null internally as part of the language spec.
jmucchiello
-1 Not using OOP
Partial
+1 to cancel the downvote. Forgetting an ampersand doesn't merit a downvote.
Kristo
sbi
@sbi: I like how you are commenting on my bad style when the author is obviously using new and delete incorrectly if he's going for style in C++.
Hooked
@Hooked: I'm glad you like it.
sbi
A: 

Pointers pointing to nowhere, should point to nowhere. pointer = null;

if (pointer != null) {
delete[] pointer;
pointer = null;
}
Havenard
You don't need the if. delete and delete[] check for null internally as part of the language spec.
jmucchiello
Which is not the case of free() or VirtualFree(). I rather explicit set it to null isntead of expecting occult and unsure behaviors.
Havenard
+1  A: 

Check this link out at the CERT Coding Standard Site. After freeing a variable you should set the pointer back to NULL since its not really pointing to anything anymore.

KFro
+3  A: 

delete on a bad pointer results in Undefined Behavior. You're lucky that it threw an exception, but you certainly shouldn't rely on that happening. To prevent it, simply set the pointer to 0 after the delete. It's safe to delete a null pointer (in case FreeGlobal() gets called more than once), so you don't have to do any if-test.

void FreeGlobal()
{
    delete [] pointer;
    pointer = 0;
}
Kristo
-1 For not using OOP.
Partial
+1 For answering the question directly, without saying something wrong (like saying "delete [] NULL" would be OK or something along that way) and for saying that the exception throwing was just random.
Johannes Schaub - litb
@Partial ...huh? The program uses a global pointer to a dynamically allocated object. This is the right way to delete the object. What's "not OOP" about this? BTW, you do realize that C++ supports programming paradigms other than OOP, do you not?
Dima
@Dima: Yes, I do realize that it does support other paradigms than OOP. Of course this answer works but there are better ways of doing so! Why use C++ if you are not going to use OOP, generic programming and data abstraction? Have a look at Bjarne Stroustrup's site: http://www.research.att.com/~bs/C++.html
Partial
Also, you should put the `-1` onto the questioner's whatever, i think. This answer just answered the issue, it didn't say "don't use OOP" and it neither said "use global functions". And in any case, i prefer bug-free simple code to bugged "better" code :)
Johannes Schaub - litb
@litb: Where is it "bugged"?
Partial
I prefer elegant and optimized code to simple and dangerous code.
Partial
If code is not simple, it cannot be elegant. And if code is simple, it is much less likely to be dangerous. The danger is in the complexity.
Dima
@Dima: Perhaps you should be programming in another language than C++ such as C# or Java that does all the complex stuff for you. C++ is meant to allow a lot of liberty. You are responsible to make safe code. Since every function has access to global variables, it becomes increasingly hard to figure out which functions actually modified these. Why are you saying that global variables are simple and not dangerous? I really do not understand your point of view.
Partial
+3  A: 

You don't need to check if the pointer is null before calling delete or delete[]. Your function should look like this:

void freeGlobal(char*& ptr) {
    delete[] ptr;
    ptr = 0;
}

What I want to know is why this thing is global and not inside a class?

class buffer {
    char* buf;
    size_t size;
public:
    buffer(size_t n) : size(n), buf(new char[size]) {}
    ~buffer() { delete[] buf; buf = 0; }

    operator char*() { return buf; }
    char& operator[](size_t ofs) {
        assert(ofs >= 0 && ofs < size);
        return buf[ofs];
    }
};

There are probably a couple things wrong with my implementation as I just typing it in here. Another question is why you aren't using the std::string for this char buffer?

jmucchiello
+1 Taking advantage of C++'s Object Oriented Programming design is the best thing to do. In your situation, the class would have the responsibility of cleaning up on itself with the destructor.
Partial
+1. But there is still an issue: That will yield an ambiguity error if you do `char c = b[0];`, because of `op[](char*,ptrdiff_t)` (built-in operator) vs `op[](buffer }` instead of the conversion function would be a better idea, i think.
Johannes Schaub - litb
Why not just use `std::vector`?
GMan
@GMan: +1 That would make it a lot easier. On the other hand, perhaps bugboy wants to do a specialized array or just learn how it works.
Partial
In fact why not simply use a string?
Partial
While the idea is good, that `buffer` class has a very serious flaw: If you (accidentally) copy such a buffer, `delete[]` will be called twice with the same pointer, invoking undefined behavior. If you're lucky, this blows up right away. If you're unlucky, it just messes up heap management and blows up later, when nothing wrong was done. Hours of fun finding such bugs... _So what's the advantage of a quickly home-brewn buffer class over something tested and proven?_
sbi
Make the copy constructor private. I said there were flaws in the implementation. It was just an example. I also said to just use std::string.
jmucchiello
SBI: The advantages of any buffer class over a raw pointer should be obvious. The advantage of a well thought out home-brewn buffer class over an existing class is you only code what you need, reducing bloat. (How big is the C++ Hello World executable these days?) Of course you asked about a quickly made class. There is no advantage there except that over time the quickly made class will eventually be tested and proven and at that point will still have the advantage of reducing bloat. Finally, every C++ coder should at some point code a real buffer class. It is educational.
jmucchiello
@jmucchiello: I wasn't suggesting using raw pointers. There's `std::vector`, 'boost::scoped_array', and many others that are well tested and proven. It's very unlikely someone will come up with something better, but rather likely they come up with something worse. How does it help your customers if your executable is a few bytes smaller (_if_ it is at all), but crashes?
sbi
SBI: No, the OP was asking about raw pointers. Obviously, if he didn't think to wrap it in a class, he wouldn't know std::vector, boost::scoped_array or any of the other well-tested and proven things exist. There is obviously a need for education here. And the best educational action is to roll your own. Knowing how to do it is more important than being able to drop in a black box. The point is not to learn how to make a buffer class but to learn C++ idioms that will serve when there is not an existing solution. People come here to learn as well as to get solutions.
jmucchiello
@jmucchiello: The best thing to fix the OP's code would have been to simply use some off-the-shelf solution. Therefor a good answer would advice to do so. Rolling your own has the huge disadvantage that you might fail. You've just shown this. /Using/ well-tested code is much easier than /creating/ it. It can therefor be done long before someone is able to create such code. That's another reason this should be the default advice. If he neither knew `std::vector` nor RYO buffers, then that's one more reason to point at `std::vector`.
sbi
SBI: We are going to have to agree to disagree. If he is a student I must stand by what I've said. Rolling it himself is the best answer. Easiest is not a good criteria when one is learning. You learn far more about C++ failing a do-it-yourself class than you do by plugging in library code. If someone is paying him to code, then you are right. He better use something off-the-shelf and start reading some books on C++ as well. I usually assume people come here to learn something. Causing problems for copy/paste folks doesn't bother me if helping them causes learners not to learn.
jmucchiello
@jmucchiello: I have been teaching C++ for several years. I set out as you say: Let them do everything themselves and then let's sit together and analyze what went wrong and why. Guess what? Doesn't work. C++ is much to big and complex a monster to be swallowed all in one bite. For me, http://www.acceleratedcpp.com was the eye-opener: You can _use_ all this great stuff without knowing the magic inside. When, doing this, you have mastered the basic techniques, you can learn how to actually implement that magic. I taught this successfully for almost a decade now.
sbi
SBI: Going way off topic now but again we still need to agree to disagree. I'll give the website a look later but I suspect it will not be my cup of tea. I have run into too many folk who think they know C++ who don't understand pointers. Whitewashing the low-level stuff doesn't do anybody any favors. I applaud your success but since I can't actually verify it I must remain skeptical.
jmucchiello
@jmucchiello: The website is for a book. And teaching the differences between object, reference, or pointer is among the first few lessons in my curriculum. I'm fine with disagreeing, though. `:)`
sbi
SBI: I haven't read that book. It has a lot of good reviews so perhaps there is some merit to its approach. But if you read the bad reviews, most of them say the book is not good for a beginner. That it is a better approach for someone with some experience. I can't comment on the book specifically but in my experience good programmers learn by doing and more importantly they learn by failing. There are too many programmers who prefer instant results over understanding. You are teaching to your audience. It is disheartening that you had to give up your first approach.
jmucchiello
@jmucchiello: The book has indeed a very steep learning curve. Form most beginners, it's a good book to accompany a C++ seminar, but hardly good to teach yourself C++. (Although I have seen a few manage that.) I'm not opposed to learning-by-doing (where I taught, students had as many lab time as lecture time) and neither is the book. Still, C++ is too big and too entangled to teach from the ground to the top. [...]
sbi
[...] One example: In order to teach all of exceptions, students need to know about inheritance and polymorphism. However, in order to properly use polymorphic classes in general, they have to use `new`, which, to teach properly, requires knowledge about RAII -- and then we're back to teaching exceptions first because that's why RAII is necessary. [...]
sbi
[...] My approach is to go in circles and re-visited subjects already dealt with, but on a higher level and thus slowly spiraling upwards. On this way, students will for a while use magic items without understanding them until later -- but that's the way it is. So they will use `std::vector` (RAII) instead of naked arrays from day one, and learn about its innards later. I vastly prefer this to the other way around. Also, if you think about it: What's the percentage of C++ programmers who coul dcome up with a bullet-proof `std::vector` implementation from scratch? Or any other API, FTM. [...]
sbi
[...] We use a lot of magic everyday. It makes our jobs easier and is the ultimate precondition to allow us to do what we do. Yes, we could come up with much of this magic ourselves -- but not in our life-time. It usually suffices for us to know how things work "in general", that we could do it "in principle", and that we can always dig into it later, when we need to. (Here endeth my rant.)
sbi
Don't worry, I enjoy a good rant. I would disagree that learning about new and learning RAII are linked but that's a bit esoteric for this comment system. Regardless, I can't disagree with the idea that C++ is big (nor that C++0x is gargantuan). I'm glad to hear the spiral (methodology) works for you. Was I advocating a waterfall method? Dear me. :-)
jmucchiello
`:)`
sbi
+4  A: 

I would suggest you actually fix your code. Double deleting something is a terrible thing to do. Rather than make hackish routines to let this happen, fix the real problem: no more double deletes.

Find out why you're deleting something twice, and stop it from happening. This would probably be easier if you weren't using global variables. If you need a global resource, use a singleton.

Additionally, use a std::vector<char>, or boost::array<char, 100>, so you don't need to worry about memory. You can't accidentally delete something twice if you don't have to worry about (or access to) deleting it.

GMan
I suspect I'll get downvoted for this because it doesn't really answer his question, but I felt it needed to be said.
GMan
+1 Your absolutely right!
Partial
no no. Thanks. I asked "is this a bad practice?".
bugboy