Replacing delete in C++, missinformation

Question

I'm trying to (and have solved) a problem with 16 byte alignment issues with a class that contains SSE optimised members. But what is bugging me is a large portion of the examples I have found online contain a line of code that to me seems totally redundant yet is repeated in many places.

public:
void* operator new (size_t size)throw (std::bad_alloc)
{
    void * p = _aligned_malloc(size, 16);
    if (p == 0)  throw std::bad_alloc();
    return p; 
}

void operator delete (void *p)
{
    Camera* pC = static_cast<Camera*>(p);
    _aligned_free(p);
}

The line in question is

Camera* pC = static_cast<Camera*>(p);

As pC is never referenced and goes out of scope at the end of the function, what is the point of doing it? I have tried taking the line out and it appears to make no difference at all yet that line appears in lots of examples! Am I missing something really obvious or has an anomalous line of code been copied from example to example blindly and become prevalent across a lot of "tutorials"?

One reason why I might do something like that is to assist debugging. With a breakpoint on the '_aligned_free(p);', I could then inspect the Camera* pC object. — Martin James, Jul 25 '12 at 08:50
If the example is always `Camera` then certainly it has been copied from tutorial to tutorial. Blindly or otherwise. — Steve Jessop, Jul 25 '12 at 08:51
No, Camera is my own class - replace it with whatever the class name was in each tutorial :) — Stuart Eagles, Jul 25 '12 at 08:53
@Martin: true, but if that's the intention of the line in the tutorial then the tutorial should probably say so. Particularly because a beginner might not immediately realise that the "object" to which `pC` points has already been destructed, and therefore won't look quite as they might expect. — Steve Jessop, Jul 25 '12 at 08:54
Overloading global `operator new` and `operator delete` is almost certainly not a good idea for this. Also, note that your `operator new` is broken since it should call all `new_handler`s in turn before throwing. /EDIT: forget the first part of the comment, I didn’t notice that this was inside a class. The second part of the comment still holds. — Konrad Rudolph, Jul 25 '12 at 08:54
@StuartEagles: maybe you should link to 3 or 4 of these many places that contain the code. It's probably easier to comment on that whole section of the tutorial than on a few lines of code that, I agree, appear incongruous. As Konrad says, that `operator new` is dodgy anyway, so it might even turn out that it's best to find another tutorial. — Steve Jessop, Jul 25 '12 at 08:57
While pointless and confusing IMHO, it's not unusual for people to write code like this as a (dubious) form of documentation... emphasising what the pointer really points to. Sometimes people dealing with `void*` like the uniformity of starting every function by casting it to the real type, hoping it'll make it easier to quickly insert meaningful operations like a line of trace. — Tony Delroy, Jul 25 '12 at 08:58
To Martin James comment: I think that you will need another statement AFTER _aligned_free(p); to debug it efficiently - with breakpoint on _aligned_free(p); the call is not executed yet, after "step over" you are exiting the block, so I would expect Camera* pC to be out of scope already. — Germann Arlington, Jul 25 '12 at 09:06
@KonradRudolph It's not required of allocation functions to use the installed handlers as an error strategy (although it is mentioned). What is mandatory is to either return `nullptr` (and be no-throw), or throw an exception (catchable via `std::bad_alloc`) in case of failure. — Luc Danton, Jul 25 '12 at 09:09
Thanks for the help guys - I have had a look at things and am not specifying my own global handlers, the only reason I was replacing the new operator was to force Camera to be 16 byte aligned to allow XMMATRIX members to be correctly aligned when Camera is dynamically allocated. Didn't expect this to become such a lively discussion! — Stuart Eagles, Jul 25 '12 at 09:30
@GermannArlington - you may well be right, especially with optimization on :( — Martin James, Jul 25 '12 at 09:51
@GermannArlington I think it was more to allow you to check that any class members that are supposed to be freed by the destructor had correctly been destroyed rather than checking that _aligned_free(p) had correctly deallocated the assigned memory. At least that was the way I interpreted it. — Stuart Eagles, Jul 25 '12 at 10:26

score 2 · Accepted Answer · answered Jul 25 '12 at 10:07

The object ends its lifetime as soon as the destructor is entered, so you cannot do much with this pointer. The line Camera* pC = static_cast<Camera*>(p); can be removed safely and the only reason it exists in tutorials is that many people just copy-n-paste the code here and there without actually thinking how it works.

The clean and correct code for your delete() will look as follows:

void operator delete (void *p)
{
    _aligned_free(p);
}

iammilind · Answer 2 · 2012-07-25T09:59:19.773

As discussed in many comments to your question, the following line is indeed redundant:

Camera* pC = static_cast<Camera*>(p);  // Just an unused variable

Even if the p was of type Camera* before (other possibilities are the subclasses like Canon*, Sony*, Nikon*), you still cannot do much with pC, because the Camera::~Camera() should have already been called. operator delete is called after that.

P.S.: I haven't come across such practice to cast a pointer to specific class, but if you encounter such advise in a tutorial then you may want to change it.

Replacing delete in C++, missinformation

2 Answers2