Evil samples of subtly broken C++ code

Question

I need some samples of bad C++ code that will illustrate violation of good practices. I wanted to come up with my own examples, but I am having a hard time coming up with examples that are not contrived, and where a trap is not immediately obvious (it's harder than it seems).

Examples would be something like:

1. Not defining copy constructor for classes with std::auto_ptr members, and using std::auto_ptr members with forward-declared classes.
2. Calling virtual functions from a constructor or a destructor (directly or indirectly).
3. Overloading a template function.
4. Circular references with boost::shared_ptr.
5. Slicing.
6. Throwing exceptions from C callbacks (directly or indirectly).
7. Floating point comparison for equality.
8. Exception safety of constructors with raw pointer members.
9. Throwing from destructors.
10. Integer overflow when compiled on different architectures (mismatch of size_t and int).
11. Invalidating a container iterator.

...or any other evil thing you can think of.

I'd appreciate some pointers to existing resources, or a sample or two.

Answer 1

The most vexing parse is an amazingly counterintuitive result of the way C++ parses things like this:

// Declares a function called "myVector" that returns a std::vector<float>.
std::vector<float> myVector(); 
// Does NOT declare an instance of std::vector<float> called "myVector"

// Declares a function called "foo" that returns a Foo and accepts an unnamed
// parameter of type Bar.
Foo foo(Bar()); 
// Does NOT create an instance of Foo called "foo" nor creates a Bar temporary

// Declares a function called "myVector" that takes two parameters, the first named
// "str" and the second unnamed, both of type std::istream_iterator<int>.
std::vector<float> myVector( 
    std::istream_iterator<int>(str),
    std::istream_iterator<int>()
);
// Does NOT create an instance of `std::vector<float>` named "myVector" while copying
// in elements from a range of iterators

This will surprise just about anybody who is not familiar with this particular quirk of the language (myself included when I started learning C++).

Answer 2

#include <iostream>

class Base
{
    public:
        virtual void foo() const { std::cout << "A's foo!" << std::endl; }
};

class Derived : public Base
{
    public:
        void foo() { std::cout << "B's foo!" << std::endl; }
};

int main()
{
    Base* o1 = new Base();
    Base* o2 = new Derived();
    Derived* o3 = new Derived();

    o1->foo();
    o2->foo();
    o3->foo();
}

And the output is:

A's foo!
A's foo!
B's foo!

Not sure if it has a name, but it sure is evil! :P

Answer 3

Code that are not exception safe can fail in ways that are not obvious to the readers of code:

// Order of invocation is undefined in this context according to the C++ standard.
// It's possible to leak a Foo or a Bar depending on the order of evaluation if one
// of the new statements throws an exception before their auto_ptrs can "own" it
accept_two_ptrs(std::auto_ptr<Foo>(new Foo), std::auto_ptr<Bar>(new Bar));

void MyClass::InvokeCallback(CallbackType cb)
{
    Foo* resource = new Foo;
    cb(resource); // If cb throws an exception, resource leaks
    delete resource;
}

Answer 4

This one came up earlier tonight. As @Billy ONeal pointed out on that post, looping on an input stream, checking only for eof(), can result in an infinite loop if an error occurs on the stream. good() should be used instead.

BAD:

while( !cin.eof() ) {
   getline(cin, input);
}

OK:

while( cin.good() ) {
   getline(cin, input);
}

---

[credit: @James McNellis]

EXCELLENT:

while (std::getline(std::cin, input)) {
}

Answer 5

Overloading the assignment operator but not handling self-assignment correctly.

Answer 6

What do you think the program will print?

#include <iostream>
using namespace std;

struct A {
    void f(int) { cout << "a" << endl; }
};

struct B: public A {
    void f(bool) { cout << "b" << endl; }
};

int main() {
    B b;
    b.f(true);
    b.f(1);
    A* a = &b;
    a->f(true);
    return 0;
}

Answer: b, b, a! The first printout is obvious. The second one is b because the definition of B::f(bool) hides the definition of A::f(int). The third one is a because overload resolution happens on the static type.

(source: Guru of the Week, but I can't find the article.)

Answer 7

Argument-Dependent Lookup (ADL, also called Koenig lookup) is not well understood by most C++ programmers and can cause some very unusual results, most notably when combined with templates.

I discussed one major pitfall of ADL in an answer to What are the pitfalls of ADL?

---

There is a lot of complexity involved in overload resolution. Problems frequently arise when using directives are used at namespace scope, especially using namespace std, since that namespace has a large number of entities with common names.

Here are two more recent examples of the using namespace std causing problems:

• distance calculation error in c++
• why didn't the positive terms get displayed in this asbolute program

Answer 8

This one, IMHO, is tricky too:

class Base {
int _value;

public:
    Base() {
        _value = g();
    }

    virtual int f() = 0;

    int g() { return f(); }
};

class Derived: Base {   
public:
    Derived(): Base()
    { /* init Derived */ }

    int f() { /* implementation */ }
}

your code will crash because of pure virtual method f() not implemented. The obvious reason is that Derived is not yet complete in the constructor, so you will end up calling the virtual pure f() and won't be detected by the compiler (usually, compiler complains if a pure virtual is invoked inside a constructor).

Anyway, it may happens that a virtual pure is invoked if you have complex constructor which invokes other member function and you don't have unit-tests in place.