Wrapper code #1

Extending, debugging, or even using legacy code comes with gotchas. I’ve been very lucky in my current job that I’ve been most doing new development for the past year and a half, but that doesn’t mean that the work is free of legacy code. We have preferred tools and a substantial internal library that dates back for decades.

For GUI’s we use Qt. This is not a rant about Qt (though I have several thing to say about it): the framework does what it is suppose to do and is well supported. But it has a lengthy legacy of it’s own and that shows through in places.

Low-level exposure

The place I want to talk about today is the test framework in general, and the output of the comparison macros QCOMPARE in particular. The message delivered to the user when a comparison test fails depends on the type. For some types (mostly built-in types and Qt’s library types) the message nicely shows you the values that were compared. For other type it just tells you that there was a failure and leaves you guessing.

Thankfully there are facilities for enabling the helpful messages on new types. (There are also facilities for providing the macro with a comparison more sophisticated than just operator== if you feel the need.

But there is a gotcha that is easy to overlook if you rely on helpful posts on the internet rather than Troll Tech’s documentation. (Which I do at times because to my mind the official documentation is consistently crystal clear after you understand the system but quite unclear when you first approach a new feature or system.)

The issue is that you are instructed to use toString to implement the pretty-printer and that appears to be an old and low-level facility. If you read the documentation you see

Note: The caller of toString() must delete the returned data using delete[].

In other words, somewhere in the bowels of the library, QTest is calling new[] to construct buffers to hold these strings and then they are returning the pointer that results directly to the caller. In 2020, writing code that exposed uses of new[] to external users like that would be a bit questionable (or at least it would require thought and justification), but that’s legacy code for you.

BTW, there is more to the note than I reproduced here. I’ll talk about the rest later.

Naive approach

could just live with this. Every time you needed to use toString you could write a little cluster of lines similar to

char * element = toString(value);
accumulatingString += '(' + element;
delete[] element;

That’s functional and it is exactly what I’d do if I were only calling toString once or twice in my entire code base. But if you called the library routine many times it would clutter up your code something fierce, and it would be easy to forget it somewhere. A more systematic approach is called for.

A wrapper class

Let’s construct a wrapper that automatically takes care of cleaning up.

Version 0: plug the leak

The purpose of the class is to manage the memory for us. We construct a class that captures the pointer and calls delete[]in it’s d’or. This is straight-forward

class StringCleaner //v0
{
public:
    StringCleaner(char * newedbuffer): m_ptr(newedbuffer) {}
    ~StringCleaner( delete[] m_ptr; )
private:
    char * m_ptr;
};

Then every time you call toString you wrap it up with our class

StringCleaner(toString(value));

which creates an temporary object that deallocates the memory when the compile destructs in at the end of it’s use. Nice.

Aside: I’ve tried so diligently to avoid using bare pointer in my code in recent years that actually manually cleaning up allocations in a d’tor like that feels like a return to my wild and irresponsible youth. Whee!

Version 1: make it useful

Alas, in version 0 we can’t use the string, which just wont do. We need to add some kind of access. You might consider a method like char * string() const; but the access syntax is icky:

StringCleaner(toString(value)).string()

So I prefer to use the * operator (reading the semantics like “look at” similar to the way you use it for an iterator or a pointer). We also want the returned value to be safe, so we return an object. That makes our class

#include "QString"
class StringCleaner // v1
{
public:
    StringCleaner(char * newedbuffer): m_ptr(newedbuffer) {}
    ~StringCleaner ( delete[] m_ptr; )
    QString operator*() const { return m_ptr; }
private:
    char * m_ptr;
};

and we use it with syntax like *StringCleaner(toString(value)).
For most purposes I prefer to work with and return standard library types over framework types, but the context for this is entirely within the framework, so using QString is natural (and as we’ll see it has a specific advantage here).

This is effectively what I did to solve the problem for work.

Version 2:

Finally, as things stand, we’re planning to call toString every time we use this routine. If we are willing to write templated code (and in this case you’re creating little mystery and adding quite modestly to the compile time, so go for it…), we can make that part of the class.

#include "QString"
#include "QTest"

template <typename T> class SafeToString // v2
{
public:
    SafeToString(const T arg) : m_ptr(toString(arg)) {}
    ~SafeToString() { delete[] m_ptr; }
    QString operator*() const { return QString(m_ptr); }
private:
    char * m_ptr;
};

which is simply used in place of toString.

Conforming to the API

The part of the note I elided above reads

Your implementation should return a string created with new[] or qstrdup(). The easiest way to do so is to create a QByteArray or QString and calling QTest::toString() on it (see second example below).

In other words if you are writing your own version of toString (which you are if you started wanting to get pretty printed fail messages when you use QCOMPARE with your own types), you need to duplicate the “mistake” of the API or risk some internal code calling delete[] on a buffer that belongs to an object of some kind with unpredictable (but generally bad) consequences.

Let us assume we’re trying to write a pretty-printer for a 2D lattice offset (so, an integer “vector”) called IVector. (I’m doing integers here to avoid having to discuss if you want and how you get approximate floating-point comparison for a compound type with QCOMPARE.) We’ll assume that IVector implements methods X and Y to access the components and has a working operator== which QCOMPARE can use. Finally, we want it to pretty-print as (x, y).

Following the advice in the note we write

char * IVector::toString()
{
    QString out = "(" + SafeToString(X()) + ", " + SafeToString(Y()) + ")";
    return toString(out); // Not safe, we need to return the new[]'ed buffer
}

We’ve cleaned up the allocation for each integer, and returned a single allocation for our string as required.