I'm starting to suspect the only way to get anywhere in this industry is to be utterly incompetent.

Case in point: Staff level engineer, which is above Senior and about as close to an executive level engineer as you can get without putting a hit out on someone. Anyway, he had a code review for a random crash during the cleanup of a C++ class. One of his fixes resulted in him deleting an object in memory, then not setting the class variable to NULL afterwards. Since it's possible for later calls to try and refer to this object to see if it's valid or not, it needed to be set to NULL.

So after he was called on it, he checked in some code that set the pointer to NULL then called delete on it.

When he got called on that he removed the delete call, as it's 'superfluous'. I've asked him to explain, and am looking forward to the reply.

And before you ask, no, he's worse at C# and Managed C. It's frightening that anyone with his supposed 20 years of experience thinks that the correct way to hide text on a UI is to set the text to the same color as the background of the window, or their method of 'disabling' a pulldown menu is to stick a disabled text box over it.

Yet he makes probably twice what I do, and gets to leave every day at 4. In fact, I've never seen him here after 4, even during crunch periods, and never on weekends. Not that I'm complaining, we get more work done when he's gone, but I know he'll get a bonus this year and the rest of us won't, because he's "senior or above".

Why no, I'm not bitter. Not at all.

Obviously the delete call was superfluous: deleting a NULL pointer does nothing. ;-)

 Obviously he's forward-thinking and will be adding multithreading. Have a dedicated "deleter" thread, set the pointer to NULL in one, simultaneously instruct the deleter to read the pointer value and delete the object in the other and the delete will happen before the core on which you set the pointer to NULL has had time to flush its L1-cache to memory, so the deleter thread reads the old value and deletes that.

Because that would be less WTFy.

Evo:

Obviously the delete call was superfluous: deleting a NULL pointer does nothing. ;-)

Ok, I'll give you that. :)

CodeNinja:

Yet he makes probably twice what I do, and gets to leave every day at 4. In fact, I've never seen him here after 4, even during crunch periods, and never on weekends. Not that I'm complaining, we get more work done when he's gone, but I know he'll get a bonus this year and the rest of us won't, because he's "senior or above".

I've had one colleague who was a demonstrably, objectively terrible programmer and was getting paid 10K more than most of the rest of the team. Whenever a project was fucked up, he always seemed to be involved and always had a million reasons why it was management's fault. His code was awful to read and he had an unusually extensive level of experience in the use of his keyboard's Ctrl, C and V buttons.

When the Bad Times rolled around, he was first on the chopping block. Sometimes, in my darker moments, I admit to myself that he was only made redundant because of his higher pay, and that management were completely oblivious to what a shitty colleague and programmer he was. This wasn't even that big of a company so I can only imagine how easy it must be for this sort of poisonous person to hide under the radar in BigCorp.

Sometimes I ask myself how such a person manages to elevate themselves to such a high level of remuneration. The answer, I believe, may be as simple as "By asking for more money a lot".

 

CodeNinja:

Evo:

Obviously the delete call was superfluous: deleting a NULL pointer does nothing. ;-)

Ok, I'll give you that. :)

On a decent system, deleting NULL will crash your program, and if you're writing a driver, the whole system, just the way God has intended.

Anyway, your pain is understood. I've seen someone start making promotion by being a loud-mouthed know-it-all in meetings. His work was ok, but he wasn't really technically very savvy or anything. Yet his attitude, the fake cameradery with one of the veeps, etc., made him look good in the eyes of management, whose main frame of reference is exactly that behaviour...

TGV:

CodeNinja:

Evo:

Obviously the delete call was superfluous: deleting a NULL pointer does nothing. ;-)

Ok, I'll give you that. :)

On a decent system, deleting NULL will crash your program, and if you're writing a driver, the whole system, just the way God has intended.

Um, no. In C++ deleting a null pointer is a no-op (as is the case in C with free().) What is less forgivable is that this behaviour is the exception, rather than the rule. fclose(0) or close(-1) are undefined, for example.

GNU Pepper:

Sometimes I ask myself how such a person manages to elevate themselves to such a high level of remuneration. The answer, I believe, may be as simple as "By asking for more money a lot".

Or it's the Dilbert principle at work - though in your case it seems to have failed in the fact it didn't take him out of the workflow.

Deleting NULL crashing your program would be....a broken system. Deleting a NULL pointer is legit C++ spec.

PJH:

In C++ deleting a null pointer is a no-op (as is the case in C with free().) What is less forgivable is that this behaviour is the exception, rather than the rule. fclose(0) or close(-1) are undefined, for example.

I thought it was something like this, honestly I've not deleted a NULL pointer in so long that I don't really remember what happens. It's just become habit for me to check the pointer for NULL before deleting it, if it can be deleted in more than one location, or I suspect someone else may modify the code later.

Oh, goody, he finally replied to the review!
"If we set m_pSettings to NULL first, the freeing code for MDelete isn't executed because it checks to see if the pointer passed to it is not null. See Line 232 (DeleteFree) in MemoryManager.h."

I think he's missing the point. What's funnier is that the Project Lead, another Senior Level Engineer (great guy!), and myself are all telling him to just move his "m_pSettings = NULL;" down one line so it occurs after the delete call, and he's arguing it.

His 'fixes' also haven't solved the problem, but that's not a surprise to anyone. I'm half tempted to come in this weekend and fix the issue for him, after re-assigning the task to myself in JIRA, but I'm so far behind on my own stuff that I probably shouldn't.

CodeNinja:

I think he's missing the point.

He is. It's a classic memory leak. Are you able to use, for example, valgrind against the application concerned?

PJH:

CodeNinja:

I think he's missing the point.

He is. It's a classic memory leak. Are you able to use, for example, valgrind against the application concerned?

At the moment, no, and now that you've mentioned it I'm a little concerned since he's been put in charge of finding software for that kind of profiling. Honestly, though, I'm not horribly surprised since people who've worked with him previously on other teams called him the 'memory leak king'. Part of me wants to turn on the leak detecting functionality in the memory manager he's working on, to see if it's able to capture a leak within itself.

I should back up a little, though. Reading through some of the comments it seems like some of you believe that his comment about removing the delete fixed the crash. This isn't the case, in fact we're not really sure why he was messing with this function, as the crash actually happens in a completely different function that's sole task is to delete another pointer. He flat out just removed that delete, so that pointer isn't being deleted either, and it's pointing to a fairly large object as well. According to his comments, the reason he deleted it was because, "things were happening out of order and the pointer was invalid with a value of 0xddddddddd". After talking with a coworker about my suspicions, we're fairly certain that's a Microsoft static value that gets set when a pointer is deleted but not NULLed. It appears that the delete is being called twice, which it would be able to handle if someone just wrapped it in an 'if' test and set it to NULL after deleting. It's kind of amusing, in a stressful way, to watch the Team Lead and another Senior Engineer try to lead him to this solution, he's actually fighting back.

This type of WTF actually makes me appreciate the review process where I work, as painful as it is.

GNU Pepper:

Sometimes I ask myself how such a person manages to elevate themselves to such a high level of remuneration. The answer, I believe, may be as simple as "By asking for more money a lot".

 

 Yes. Raises are decided mainly by human contact with the HR departement usually, and even if a superioris supposed to give his technical opinion, je may not directly read his code for lack of time, of simply have a good opinion of him and zsuppose that if he is sympathic he is competent.

Once someone is senior enough, he is much less likely to getcalled on error on top of that, because people who have elevated him in the first place think they had good reason to do so. The sad thing : if you were in the shoes of the HR people or superior, you would be very likely to be tricked too. Human are wired to favor their friend, not the competent people.

 

GNU Pepper:

Sometimes I ask myself how such a person manages to elevate themselves to such a high level of remuneration. The answer, I believe, may be as simple as "By asking for more money a lot".

 

It's called "being a sub-criminal psychopath".  It's symptoms are rigorous suppression of anyone below you, and diligent attendance on anyone above. It works really well, and this personality type predominates in middle management. Occasionally people get in trouble for it (like when they are incompetent surgeons who's body count gets too high), but this happens rarely. Normally they are able to continue their destructive M.O. until they retire with huge bonuses and pensions.

 

The real WTF is spraying your code with deletes.

C++ code should be written with RAII - no that isn't an American organisation that manages music licensing (that's RIAA). It's a computing technique to manage resources and their lifetimes, executed through the lifetime of other objects that depend on them.

The only places a delete should ever be called is:

1. A destructor
2. An object or function object passed into a smart-pointer as a deleter. Thus if you are using shared_ptr with a custom deleter, your custom deleter can call delete.

The correct logic would be to have a smart pointer member of the class (probably shared_ptr or unique_ptr), and "reset" this to a null.

 

 

Cbuttius:

The real WTF is spraying your code with deletes.

 

+1

 

PJH:

Um, no. In C++ deleting a null pointer is a no-op (as is the case in C with free().) What is less forgivable is that this behaviour is the exception, rather than the rule. fclose(0) or close(-1) are undefined, for example.

I know, that's why I wrote "decent": 'cause it should, like in C, where, at least in the olden days, free(NULL) would cause a bus error. Freeing NULL just isn't right. I haven't checked the ANSI spec on this issue, but my guess is it's probably still valid behavior.

TGV:

PJH:

Um, no. In C++ deleting a null pointer is a no-op (as is the case in C with free().) What is less forgivable is that this behaviour is the exception, rather than the rule. fclose(0) or close(-1) are undefined, for example.

I know, that's why I wrote "decent": 'cause it should, like in C, where, at least in the olden days, free(NULL) would cause a bus error. Freeing NULL just isn't right. I haven't checked the ANSI spec on this issue, but my guess is it's probably still valid behavior.

As stated by PJH:

PJH:

Um, no. In C++ deleting a null pointer is a no-op (as is the case in C with free().) What is less forgivable is that this behaviour is the exception, rather than the rule. fclose(0) or close(-1) are undefined, for example.

I know this to be true as well. While I'm too lazy to look it up in the standard right now, here's a copy of a line from the free man page:

man free:

The free() function deallocates the memory allocation pointed to by ptr. If ptr is a NULL pointer, no operation is performed.

PJH:

GNU Pepper:

Sometimes I ask myself how such a person manages to elevate themselves to such a high level of remuneration. The answer, I believe, may be as simple as "By asking for more money a lot".

Or it's the Dilbert principle at work - though in your case it seems to have failed in the fact it didn't take him out of the workflow.

You're confusing your terminology there.

Dilbert principle is about position elevation (promotion) which often brings about a change in responsibilities. It sounds like GNUPepper was talking more about financial elevation (pay raise) but retaining the same position.

It's an interesting slant on the Peter Principle, too: this individual isn't just promoted to their level of incompetance, but have managed to command a high level of remuneration. I'm sure there's a term used for people that stay in the same vocation but bounce between companies, picking up pay increases whilst not actually changing their fundamental duties (and thus aren't required to gain new skills).

Evo:

As stated by PJH:

PJH:

Um, no. In C++ deleting a null pointer is a no-op (as is the case in C with free().) What is less forgivable is that this behaviour is the exception, rather than the rule. fclose(0) or close(-1) are undefined, for example.

I know this to be true as well. While I'm too lazy to look it up in the standard right now, here's a copy of a line from the free man page:

man free:

The free() function deallocates the memory allocation pointed to by ptr. If ptr is a NULL pointer, no operation is performed.

For that matter, from GCC's manpage:

       -fdelete-null-pointer-checks
           Use global dataflow analysis to identify and eliminate useless
           checks for null pointers.  The compiler assumes that dereferencing
           a null pointer would have halted the program.  If a pointer is
           checked after it has already been dereferenced, it cannot be null.

           In some environments, this assumption is not true, and programs can
           safely dereference null pointers.  Use
           -fno-delete-null-pointer-checks to disable this optimization for
           programs which depend on that behavior.

           Enabled at levels -O2, -O3, -Os, -Oz (APPLE ONLY).
…
       -ftree-vrp
           Perform Value Range Propagation on trees.  This is similar to the
           constant propagation pass, but instead of values, ranges of values
           are propagated.  This allows the optimizers to remove unnecessary
           range checks like array bound checks and null pointer checks.  This
           is enabled by default at -O2 and higher.  Null pointer check
           elimination is only done if -fdelete-null-pointer-checks is
           enabled.

Apparently repeated pointless incantations to keep from accidentally touching a NULL pointer are so common GCC's learned to remove them. I wonder if it's smart enough to remove pointless NULL checks before free/delete too…

Enterprise Architect:

Enabled at levels -O2, -O3, -Os, -Oz (APPLE ONLY).

I had never heard of -Oz before (and can't quickly find any reference to it...did you make this up?), but it seems like an appropriate choice for Apple specific optimizations.

Cbuttius:

The real WTF is spraying your code with deletes.

C++ code should be written with RAII - no that isn't an American organisation that manages music licensing (that's RIAA). It's a computing technique to manage resources and their lifetimes, executed through the lifetime of other objects that depend on them.

The only places a delete should ever be called is:

1. A destructor
2. An object or function object passed into a smart-pointer as a deleter. Thus if you are using shared_ptr with a custom deleter, your custom deleter can call delete.

The correct logic would be to have a smart pointer member of the class (probably shared_ptr or unique_ptr), and "reset" this to a null.

 

 

While I agree with you in theory, there are certain times that the use of a Smart Pointer isn't worth the performance overhead. For example, realtime rendering applications running across multiple synced computers. We've got quite a few smart pointers in use though, actually, I believe 95% of our delete calls are actually in destructors or smart pointers. Typically if something is newed it's kept around until object deletion or the object state is reset to initial.

This particular instance is in the Shutdown of the memory management subsystem, which is called either during destruction or anytime the API is told to recycle to it's origin state. So it's not like it happens often. In fact, the bug he's fixing is pretty much, "Shutting down this tool results in the application hanging and requiring a CTRL+ALT+DEL to close". It's flagged as Critical, but it's not actually stopping anyone from doing work.

Aaaaand we enter Iteration 3 (starting week 5, 2-week iterations) with him getting huffy and saying, "Fine, since this didn't fix the problem I'll just start all over again from the beginning". He also didn't take any new tasks in this morning's iteration planning, so this is the only task he has for 2 weeks.

CodeNinja:

According to his comments, the reason he deleted it was because, "things were happening out of order and the pointer was invalid with a value of 0xddddddddd". After talking with a coworker about my suspicions, we're fairly certain that's a Microsoft static value that gets set when a pointer is deleted but not NULLed.

It's a feature of the CRT debug heap. The runtime overwrites freed memory with 0xDD (more details). Therefore, if a pointer is 0xDDDDDDDD, the most likely cause is that the object containing the invalid pointer has already been destroyed and you should be looking for dangling pointers to that object instead.

Agreed. Not sure where we stand with it right now, I'm taking a hands off approach to see if it gets fixed. I don't want to take the guy's only task for a two-week iteration away from him, you know? I'd feel bad.

Oh, hey, cool he checked in more code.

Less changes this time, some of it looks promising. Proper pointer testing and deleting, although I'm curious as to why he removed some of the null checks.

And he's still spelling my name wrong, even though I've told him twice how to spell it. It's got less than 6 letters, it's not hard!

CodeNinja:

It's got less than 6 letters, it's not hard!

I count 9!

CodeNinja:

It's got less than 6 letters, it's not hard!

Harde?

boomzilla:

Enterprise Architect:

Enabled at levels -O2, -O3, -Os, -Oz (APPLE ONLY).

I had never heard of -Oz before (and can't quickly find any reference to it...did you make this up?), but it seems like an appropriate choice for Apple specific optimizations.

If you compile with -Oz, your Hello World program will print "G'day, mate!".