I use my SAFE_DELETE macro pretty much anywhere I destroy stuff. So tell me, why is it wrong to use this macro?
Some reasons listed up, point 1 to 3 have already been mentioned. Many of the arguments refer to macros in general (not or not only yours).
1. if(p) delete p;Checking for null pointers before delete is useless. As delete performs the comparison itsself, you don't gain anything by doing it a second time.
2. p = NULL;Resetting the pointer to null is not obvious from the macro name. What does "safe" mean? Now you know it, but what about other people looking at your code, or even you after you haven't worked at this project for a while?
3. p = NULL;It's probable, that the functionality to reset the pointer after deallocation is not even required, for example in a destructor. Not only that you are wasting few performance, this can be a source of error. Yes, you heard right, resetting to null is not always making code safer.
The reason is: The preemptive null-assignment nicely hides logic errors. Let's assume you deleted a pointer by mistake too early (may happen).
// code...
SAFE_DELETE(Pointer); // this should be done later for correct code
// a lot of code...
DoSomething(Pointer);void DoSomething(Element* Pointer)
{
if (Pointer)
PerformCrucialOperations(*Pointer);
}Very fine. Resetting the pointer makes the program stable, as it doesn't crash by a invalid pointer dereferencing. But - is this really wanted? The crucial operations aren't performed. It's likely you notice that only later, in a totally different function. Very funny to debug this. An instant crash, however, would have immediately shown the problem where it arises.
4. Evaluation (very important) #define SAFE_DELETE(p) if(p) { delete p; } p = NULL;You should know that the expression p is evaluated
three times, and indeed in
every case the macro is expanded. Thats's almost always bad and one of the reasons why functions should be preferred in this context.
Let's regard a more complex argument to SAFE_DELETE:
SAFE_DELETE(GetPointer()); // where GetPointer() is a slow operationAs you see, the whole slow operation is performed three times, which is absolutely unnecessary. More than that, it can even be very dangerous. Consider this example:
Element* Ptr = GetPointerBeforeElementToDelete();
SAFE_DELETE(++Ptr);
This is very evil, because the pointer is incremented three times, thus leading to undefined behaviour because checking for null, deleting, and resetting to null refer to three different addresses.
5. Operator precedenceNot very relevant here, but macros have generally got problems with priorities.
They often require to write down a forest of parentheses. Example:
#define PRODUCT(a,b) a*b
int result = PRODUCT(1+3, 2);What do you expect as result? Hint: It's
not 8.
The result is seven. That is because the macro expands to the following code:
int result = 1+3*2;Another problem functions don't have.
6. Type safetyMacros are not typesafe. Because they are just dumb text replacement, no type can be associated.
void func(double);
void func(float);
#define MYFLOAT 3.1415
func(MYFLOAT);Because the macro-definer forgot to append the "f" prefix for floats, the wrong function is invoked. This wouldn't have happened with a typesafe constant:
const float MYFLOAT = 3.1415;7. LifetimePreprocessor symbols do not survive the preprocessor. They are invisible to compiler and debugger, making error search more difficult.
