Don't use an MFC CArray with types that contain self pointers or back pointers

CArray is MFC's resizing array class. Its semantics are similar to std::vector's, and in new code you should prefer vector because of the standard library support that comes with it (and because of the gotcha I'm about to describe). But if you're working with an MFC legacy codebase or interacting with the MFC framework itself, you will at times find yourself needing to use CArray. And CArray can get you into trouble if you use it with the wrong types.

The difficulty comes from the way CArray handles the copy operation for its elements when its buffer is relocated in memory as part of a resize operation. vector and the other standard library containers copy their elements one at a time using the contained type's copy/move constructor, but CArray just does a single bulk memcpy from the old buffer space into the new one. If your type is POD this is fine, but if it defines a copy constructor, it won't be called and things will probably blow up (and Microsoft does document this on their CArray page). In particular, if there are any sort of self pointers or back pointers involved, they will become wild. For example, consider this code:

void TraceAddress(const CString& prefix, void* p)
{
 CString str;
 str.Format(_T("%s: %p\n"), prefix.GetString(), p);
 TRACE(str);
}

void Demo()
{
 struct Foo
 {
  Foo() : me(this) {}
  Foo(const Foo&) : me(this) {}
  Foo* me;
 };

 TRACE(_T("\nCArray:\n"));
 CArray<Foo> aFoo;
 
 aFoo.SetSize(1);
 TraceAddress(_T("Address of aFoo[0]"), &aFoo[0]);
 TraceAddress(_T("me ptr  of aFoo[0]"), aFoo[0].me);
 
 aFoo.SetSize(20);
 TRACE(_T("Array resized\n"));
 TraceAddress(_T("Address of aFoo[0]"), &aFoo[0]);
 TraceAddress(_T("me ptr  of aFoo[0]"), aFoo[0].me);

 TRACE(_T("\nVector:\n"));
 std::vector<Foo> vFoo;
 
 vFoo.resize(1);
 TraceAddress(_T("Address of vFoo[0]"), &vFoo[0]);
 TraceAddress(_T("me ptr  of vFoo[0]"), vFoo[0].me);
 
 vFoo.resize(20);
 TRACE(_T("Vector resized\n"));
 TraceAddress(_T("Address of vFoo[0]"), &vFoo[0]);
 TraceAddress(_T("me ptr  of vFoo[0]"), vFoo[0].me);
}

The output I saw when I ran this on my machine was:

CArray:
Address of aFoo[0]: 0063AC90
me ptr  of aFoo[0]: 0063AC90
Array resized
Address of aFoo[0]: 0063ACD0
me ptr  of aFoo[0]: 0063AC90

Vector:
Address of vFoo[0]: 0063AC90
me ptr  of vFoo[0]: 0063AC90
Vector resized
Address of vFoo[0]: 0063ADA8
me ptr  of vFoo[0]: 0063ADA8

What happened here? When we called aFoo.SetSize(1), the CArray reserved a buffer large enough to hold a single Foo at address 0063AC90. It then constructed a Foo there using the class's default constructor, which correctly initializes the me pointer. Then, when we called aFoo.SetSize(20), it reserved a new buffer large enough to hold 20 Foos at address 006ACD0 and copied the existing Foo object to this location. But instead of using the copy constructor to do this, it just memcpyied the thing, which left the me member pointing to the old address. This is a bug, but you won't discover it until the next time someone tries to use that me pointer. By then, the thread could be miles away from the resize operation leaving you with little clue how things went bad. vector, by contrast, uses Foo's copy constructor when it needs to relocate its buffer, so everything works out ok.

This case may seem obvious and contrived, but sometimes the self references are indirect and less easy to detect. For example, a co-worker recently called me over to help him with a tough, reproducible bug he couldn't figure out. A function took a const vector<float>& argument and then performed a std::max_element operation on it, but when max_element went to construct an iterator for its return value, a checked iterator failure was occurring. He was convinced he'd found a bug in the standard library, but I wasn't so sure. Tracing back a ways, it turned out that the input vector was ultimately a member of a struct that was being stored in a CArray. Something like this:

struct S
{
   std::vector<float> v;
};

CArray<S> sArray;
//...
FunctionThatFailed(sArray[0].v);

Upon seeing this, I immediately suspected that he'd stumbled into a CArray resize bug. And sure enough, it turns out that under our standard library implementation (MSVC++ 2012), vector contains a pointer to a proxy (_Myproxy) which in turn contains a pointer back to the container it proxies for (_Mycont). When that CArray got resized it broke all those _Mycont back pointers for the existing elements, but this didn't show up until much later when iterator creation was triggered by calling std::max_element on one of the individual vectors. I suggested he replace the CArray with a vector, and once he did, everything worked fine.

Dependency injection with interface-based programming: how to avoid repeating constructor arguments ad nauseum

I've already written about interface-based programming in C++ here. I haven't written specifically about dependency injection, but there's no shortage of material out there on that subject; for example here, here, and here. It's natural to want to combine the two, but in C++ at least, doing so has the unfortunate side effect of forcing you to repeat the dependency list many times. For example, consider a class with three dependencies: Foo, Bar, and Baz (for the sake of simplicity, suppose they will all be passed and stored in shared_ptrs). First, we need to list these in the declaration of the class's public factory method:

// MyClass.h
#include <memory>

class MyClass
{
public:
   static std::unique_ptr<MyClass> Create(
      const std::shared_ptr<Foo>& foo,
      const std::shared_ptr<Bar>& bar,
      const std::shared_ptr<Baz>& baz);
   // ...
};

Then again as member variables in the impl class:

// MyClass.cpp
#include "MyClass.h"

namespace
{
   class MyClassImpl : public MyClass
   {
   private:
      const std::shared_ptr<Foo> m_foo;
      const std::shared_ptr<Bar> m_bar;
      const std::shared_ptr<Baz> m_baz;
      // ...
   };
}

Then a third time as constructor arguments to the impl class, and a fourth and fifth in the constructor's initializer list (thankfully, we only need the names here, not the types):

// MyClass.cpp
#include "MyClass.h"

namespace
{
   class MyClassImpl : public MyClass
   {
      // ...
   public:
      MyClassImpl(
         const std::shared_ptr<Foo>& foo,
         const std::shared_ptr<Bar>& bar,
         const std::shared_ptr<Baz>& baz)
         : m_foo(foo)
         , m_bar(bar)
         , m_baz(baz)
      {
      }
      // ...
   };
}

A sixth statement is needed in the signature of the factory method's implementation, and finally, a seventh in that method's body when we instantiate the impl (again, with just the names this time):

// MyClass.cpp
// ...
std::unique_ptr<MyClass> MyClass::Create(
      const std::shared_ptr<Foo>& foo,
      const std::shared_ptr<Bar>& bar,
      const std::shared_ptr<Baz>& baz)
{
   return std::unique_ptr<MyClass>(new MyClassImpl(foo, bar, baz));
   // use std::make_unique with C++14
}

That's a lot of repetition. It means if you want to add or remove or just rename a dependency, you need to edit code in a minimum of seven different places.

A simple thing we can do to reduce the pain is to wrap all the dependencies up in a single struct:

// MyClass.h
#include <memory>

class MyClass
{
public:
   struct Dependencies
   {
      std::shared_ptr<Foo> foo;
      std::shared_ptr<Bar> bar;
      std::shared_ptr<Baz> baz;
   };

   static std::unique_ptr<MyClass> Create(const Dependencies& dependencies);
   // ...
};

// MyClass.cpp
#include "MyClass.h"

namespace
{
   class MyClassImpl : public MyClass
   {
   private:
      const Dependencies m_d;

   public:
      MyClassImpl(const Dependencies& d)
         : m_d(d)
      {
      }
      // ...
   };
}

std::unique_ptr<MyClass> MyClass::Create(const Dependencies& d)
{
   return std::unique_ptr<MyClass>(new MyClassImpl(d));
   // use std::make_unique with C++14
}

We still have to repeat Dependencies itself the same number of times, but it's only one thing, and more importantly, it stays the same even if we modify the dependency list. Further, it's really easy to make reusable code snippets out of something like the above (minus the actual contents of the Dependencies struct).

However, there's another problem now. When each dependency was a constructor argument, it was impossible to forget one. That isn't the case when they are struct members; for example, with the original code, the compiler would reject something like this:

auto myObject = MyClass::Create(someFoo, someBar); // oops, forgot the Baz

But with the new code, the below would build with no problems:

MyClass::Dependencies d;
d.foo = someFoo;
d.bar = someBar;
// oops, forgot the Baz
auto myObject = MyClass::Create(d);

We could handle this by validating that d.baz != nullptr inside MyClassImpl's constructor, but that wouldn't happen until runtime. Your first thought might be to give Dependencies a constructor instead of using memberwise initialization, but then we're back to the original problem of having to repeat all the arguments over and over (though admittedly not as many times).

Another idea is to use aggregate initialization:

MyClass::Dependencies d =
{
   someFoo,
   someBar,
   someBaz
};
auto myObject = MyClass::Create(d);

This looks promising, but what happens if we forget that Baz now?

MyClass::Dependencies d =
{
   someFoo,
   someBar,
   // oops; forgot the baz
};
auto myObject = MyClass::Create(d);

Unfortunately for our current purposes, this still compiles; d.baz will simply be initialized using shared_ptr's default constructor. But there is a way to prevent this—we just need to make sure the last member in Dependencies is something that cannot be default-constructed. For example:

// FinalMember.h
struct FinalMember
{
   FinalMember(int) {}
};

// MyClass.h
#include "FinalMember.h"

class MyClass
{
public:
   struct Dependencies
   {
       const std::shared_ptr<Foo> foo;
       const std::shared_ptr<Bar> bar;
       const std::shared_ptr<Baz> baz;
       FinalMember finalMember;
   };
   // ...
};

Now callers have no choice but to initialize all the members:

MyClass::Dependencies d =
{
   someFoo,
   someBar,
   someBaz,
   FinalMember(0) // can't omit or put in wrong position without a compile error
};
auto myObject = MyClass::Create(d);

Note that FinalMember could be replaced with anything that can't be value-initialized, including a plain const & to anything, but using a named type helps communicate our intent more clearly.

Downside (maybe): You now must use aggregate initialization when you set up new instances of Dependencies. That's not so bad (and in fact it allows us to make every member const, which makes me feel all warm and fuzzy), but since aggregate initialization is not used all that often with structs, your IDE may not understand what you're doing well enough to help you put things in the right order. Also, some compilers may be uncomfortable with the fact that Dependencies has no user-defined constructors since a default one cannot be generated. For example, with Visual Studio 2012, the code above triggers 3 warnings (the last one is simply incorrect, as we've seen):

warning C4510: 'MyClass::Dependencies' : default constructor could not be generated
warning C4512: 'MyClass::Dependencies' : assignment operator could not be generated
warning C4610: struct 'MyClass::Dependencies' can never be instantiated - user defined constructor required

I have to go to the trouble of disabling these with a pragma since I build with "treat warnings as errors" enabled. But if you (and your tools) can get past this, there's a lot to be said for the convenience this approach can offer.

VC++: steady_clock is not steady

Update July 12, 2015: The problem described below is fixed in the VS2015 RC


C++11 introduced a number of new clock classes, among them system_clock and steady_clock. system_clock is meant to represent wall time; in other words, if a clock adjustment takes place or daylight saving time goes into or out of effect, it may jump forward or backward at a rate that does not reflect the actual passage of time. These properties make it ideal for reporting the actual time. steady_clock, on the other hand, is meant to always increase monotonically, making it ideal for measuring intervals between events. However, Microsoft screwed up here—as of VC++ 2013, at any rate, steady_clock jumps around with changes to the wall clock just like system_clock. I demonstrated this with the simple program below:

#include <Windows.h>
#include <chrono>
#include <iostream>

using namespace std::chrono;

void main()
{
    uint64_t gtcStart = ::GetTickCount64();
    auto steadyStart = steady_clock::now();
    auto systemStart = system_clock::now();
    auto monoStart = monotonic_clock::now();
    auto hrStart = high_resolution_clock::now();

    getchar();

    auto gtcElapsedSec = (::GetTickCount64() - gtcStart) / 1000;
    auto steadyElapsedSec = duration_cast<seconds>(steady_clock::now() - steadyStart).count();
    auto systemElapsedSec = duration_cast<seconds>(system_clock::now() - systemStart).count();
    auto monoElapsedSec = duration_cast<seconds>(monotonic_clock::now() - monoStart).count();
    auto hrElapsedSec = duration_cast<seconds>(high_resolution_clock::now() - hrStart).count();

    std::cout << "GetTickCount64(): " << gtcElapsedSec << " sec" << std::endl;
    std::cout << "steady_clock: " << steadyElapsedSec << " sec" << std::endl;
    std::cout << "system_clock: " << systemElapsedSec << " sec" << std::endl;
    std::cout << "monotonic_clock: " << monoElapsedSec << " sec" << std::endl;
    std::cout << "high_resolution_clock: " << hrElapsedSec << " sec" << std::endl;
}

While the code was sitting on getchar(), I snuck into Control Panel and set the clock ahead an hour, then returned to the program and entered a char. The output was:

GetTickCount64(): 13 sec
steady_clock: 3611 sec
system_clock: 3611 sec
monotonic_clock: 3611 sec
high_resolution_clock: 3611 sec

Now; if you're using boost in your project (and any C++ project of substance really should), boost::chrono::steady_clock has the same syntax and semantics, but actually works. Otherwise, unless you want to write your own clock, it looks like you're stuck with GetTickCount64() until the next release of Visual Studio.

A PropertyGrid gotcha

In a nutshell, when placed in a PropertyGrid, objects that use the same DisplayNameAttribute in different categories lead to surprising selection behavior.

I decline to call it a bug because I suspect the authors of the framework would claim this is how it's supposed to behave, but it's still the sort of thing that can trip developers up. Imagine you set a PropertyGrid's SelectedObject to an instance of this class:

class Demo
{
    [Category("Category1")]
    [DisplayName("Value")]
    public int Val1 { get; set; }

    [Category("Category2")]
    [DisplayName("Value")]
    public int Val2 { get; set; }

    [Category("Category3")]
    [DisplayName("Value")]
    public int Val3 { get; set; }
}

Sure, we're reusing the display name "Value," but since each instance is in a different category, everything should be fine, right? Well; not exactly. Under the hood, the grid determines equality between GridEntrys by comparing the combination of label, property type, and tree depth. And in this case, "label" means display name. So it doesn't matter if the actual code names of the underlying properties differ—as long as their display names, types, and depths are the same, the grid will consider their entries equal. One consequence of this is that if PropertyGrid.Refresh() is called while the Category3 Value is selected, the selection will move to the Category1 Value; presumably because it is the first entry in the tree where .Equals(oldSelection) returns true.

Luckily, there is a workaround. PropertyGrid omits tab characters when it prints display names, so you can simply add a different number of \t's to the beginning or end of each one to make them unique. A bit of a hack to be sure, but it does the job.

Thanks to stackoverflow for assisting me in my investigation of this issue.

A make_shared caveat

When you allocate a new object for a shared_ptr using the obvious approach:

std::shared_ptr<Foo> p(new Foo(a1, a2)); // case 1

you wind up with (at least) two dynamic memory allocations: one for the Foo object itself, and another for the shared_ptr's control block (which includes the strong and weak reference counts).

That's not usually a big deal, but it would help locality of reference if we could put the whole thing into one block, as well as making construction slightly faster. The make_shared library function can help us with that:

std::shared_ptr<Foo> p = std::make_shared<Foo>(a1, a2); // case 2

make_shared will typically place the shared_ptr control block and the allocated object itself into a single chunk of memory. I say "typically" because the standard does not require implementers to do things this way, but in practice, they do.

This seems like win-win, but there is a catch. Imagine a situation where the Foo object we created above is referenced by one shared_ptr and two weak_ptrs (i.e. its strong count is one and its weak count is two). Now that one shared_ptr goes away. If we'd allocated the Foo as in case 1, at this point we'd be able to free all the memory it used to occupy and go on our way. But if we'd done things as in case 2, this isn't possible. Why? Because we still need to keep the control block around until the weak count goes to zero, the control block and the Foo share a single allocation, and it isn't possible to free just part of an allocation. So while the Foo's destructor will be called the moment the strong count goes to zero as expected, if it was created using make_shared, its memory can't be returned to the system until the weak count does too. In situations where dangling weak_ptrs can hang around more or less indefinitely, this can lead to some surprising memory leaks.

pimpl makes move constructors easy

I described the pimpl idiom and some of its uses in a previous post. Another little fringe benefit is that it makes writing move constructors really, really easy. Consider this (partial) class:

class TuneResult
{
private:
    std::string m_name;
    std::map<std::pair<double, double>, double> m_measurements;
    std::pair<double, double> m_optimum;
    std::vector<std::string> m_logMessages;
};

Without pimpl, adding a move constructor requires a bit of effort:

class TuneResult
{
    TuneResult(TuneResult&& other)
        : m_name(std::move(other.m_name))
        , m_measurements(std::move(other.m_measurements))
        , m_optimum(std::move(other.m_optimum))
        , m_logMessages(std::move(other.m_logMessages))
    {    
    }

private:
    std::string m_name;
    std::map<std::pair<double, double>, double> m_measurements;
    std::pair<double, double> m_optimum;
    std::vector<std::string> m_logMessages;
};

It's not hard to imagine that becoming much longer for classes with more members. But if you've already pimpled anyway, moving becomes trivial:

// h file

class TuneResultImpl;

class TuneResult
{
    TuneResult(TuneResult&& other);
    
private:
    friend class TuneResultImpl;
    std::unique_ptr<TuneResultImpl> m_impl;
};

// cpp file 

class TuneResultImpl
{
    friend class TuneResult;

private:
    TuneResult* m_self;

    std::string m_name;
    std::map<std::pair<double, double>, double> m_measurements;
    std::pair<double, double> m_optimum;
    std::vector<std::string> m_logMessages;
};

TuneResult::TuneResult(TuneResult&& other)
    : m_impl(std::move(other.m_impl))
{
    m_impl->m_self = this; // don't forget!
}

C++: Tying object lifetimes together with shared_ptr and custom deleters

Once upon a time, when a great man died his slaves would be killed and buried alongside him as gifts to carry into the netherworld. Our civilizations have mostly moved beyond both slavery and grave gifts, but our code has not—it is actually quite common to have a series of subordinate helper objects that you want to keep around for exactly as long as some master object is alive. The most straightforward way of doing this, of course, is to have the the slave objects be actual member variables of the master's class. But that won't work if you've made the choice to keep your objects loosely coupled (for any of several excellent reasons). For example, consider a class that allows builders to attach custom behaviors to it using the observer pattern:

class Master
{
public:
   struct IObserver { /* ... */ };

   // WEAKLY attach an observer that will be notified when this object changes
   void AddChangedObserver(std::weak_ptr<IObserver> observer);
};

// write the changes to disk
class Serializer : public Master::IObserver { /* ... */ };

// update UI to reflect changes to the object
class UiUpdater : public Master::IObserver { /* ... */ };

std::shared_ptr<Master> MakeMaster()
{
   auto master = std::make_shared<Master>();

   auto serializer = std::make_shared<Serializer>();
   master->AddChangedObserver(serializer);

   auto uiUpdater = std::make_shared<UiUpdater>();
   master->AddChangedObserver(uiUpdater);    

   return master; // danger!
}

There is a problem here. We want serializer and uiUpdater to stick around as long as the returned Master does, but that requires us to keep a shared_ptr to each somewhere. The code is not doing that now, so both will be deleted (and implicitly deregistered) as soon as MakeMaster() returns. This will not cause any undefined behavior, but we won't get the attached behaviors we were expecting from these objects either.

So where do we keep the shared_ptrs to the two observers? We could return them from MakeMaster() along with the main object, but that's just punting to the caller (who shouldn't need to know that we're attaching behaviors anyway). A better solution is to leverage the ability to give shared_ptr a custom deleter. Instead of return master, we can do this:

std::shared_ptr<Master> master2(master.get(),
    [master, serializer, uiUpdater](Master*) mutable
    {
        master.reset();
        serializer.reset();
        uiUpdater.reset();
    });
return master2;

This requires a bit of explanation. First of all, we are creating a new reference count/shared_ptr "family" over the Master object we created earlier. Normally having more than one reference count for the same object would eventually result in a double-deletion disaster, but that won't happen here. The reason is that we're giving this second shared_ptr a custom deleter lambda that doesn't actually delete anything—instead, it simply hangs on to "keepalive" shared_ptrs to the three objects via captures. However, when the master2 family's strong reference count reaches zero and this deleter lambda is called, it will reset^* all three of them—it is this action that will result in the actual deletion of the three objects via the ordinary shared_ptr mechanism. Which is exactly what we want.

^*Note: It is important to actually reset the captured shared_ptrs in the deleter and not simply rely on them being destroyed when the lambda is. If this wasn't done, weak_ptrs in the master2 family would be able to keep the objects alive.