Category Archives: Uncategorized

Some bad code I’ve seen

This is a living post of horrid code I’ve actually encountered.

1) Assignment within an if is generally considered bad practice, but I’ve seen it in a lot of C code over the years. The issue is that it can look like a comparison operator typo and due to context it can sometimes be difficult to know which one was intended . This was the case when I stumbled across this gem in a custom wrapper for an ODBC library.

size_t hr, * c = new size_t;
...
if ((hr = (RefreshView(), GetResultCount(*c))) > 1){
...
}

2) I recently came across some ancient and misguided code that was using the CRTP wrong. The core of the issue wasn’t really the CRTP but that they were using it without really thinking through what they were using it for. Surely the developer had thought, “this is just a wrapper around a lookup type and I need fast access” but in the end they dug themselves into a hole and then wrongly assumed they could use reinterpret casts to get themselves out of it. While I agree they needed fast lookup and CRTP gave them that, using reinterpret cast on anything but a primitive should be a show-stopping, code-review-rejecting, go-review-the-basics-again sort of offense. The code they wrote was basically what follows but with lots of members in each class.

class A {};
//Although B inherits from A it has a different memory layout. 
class B : public A {};

template <typename T>
class Sub1 : public T {};

template < typename T = A>
class Sub2 : public Sub1<T> {};

//When B is propagated to the base class via CRTP it makes the memory layouts between subclasses non-covariant. 
template <typename T = B>
class Sub3 : public Sub2<T> {};

//this expects a Sub2<A> *
void Mutate(Sub2<>* b) { 
// do some things
}

//This next section was early in the execution path
...
//this is a Sub3<B> by default, if they had used Sub3<A> then they could have used a dynamic_cast further on and all would be well, but they needed the functionality of B so...
   Sub3<> sub3; 
//memory corruption here
   Mutate(reinterpret_cast<Sub2<>*>(&sub3));
...

There’ll be more to come. I expect this post will grow quickly now that I’m collecting these things.