In http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++#Adding_new_data_members_to_classes_without_d-pointer there is the following code snippet for adding a new member to classes without d-pointer while preserving binary compatibility:

Code: Select all

// BCI: Add a real d-pointer
Q_GLOBAL_STATIC(QHash<Foo *,FooPrivate *>, d_func);
static FooPrivate* d( const Foo* foo )
{
    FooPrivate* ret = d_func()->value( foo, 0 );
    if ( ! ret ) {
        ret = new FooPrivate;
        d_func()->insert( foo, ret );
    }
    return ret;
}
static void delete_d( const Foo* foo )
{
    FooPrivate* ret = d_func()->value( foo, 0 );
    delete ret;
    d_func()->remove( foo );
}

As I understand, this is a bad example since it makes any methods of Foo that will use the newly added field non-reentrant (where "reentrant" is meant according to the definition in the Qt documentation: http://qt-project.org/doc/qt-4.8/threads-reentrancy.html) because QHash is only reentrant but not threadsafe (http://qt-project.org/doc/qt-4.8/qhash.html). So if the class Foo was reentrant and some code using Foo relied on that property, it may be broken.

I also think it is a good coding practice to keep all methods and classes reentrant whenever possible, to allow multi-threaded use and avoid bugs by unwary developers (actually I am new to Qt and KDE but I have had more experience with Java, where I cannot remember seeing any non-reentrant methods in any library, except for the Swing GUI methods which must be called from the special Swing thread).

So I think that a mutex should be added to the code example in TechBase (or if not, then at least a serious warning of breaking reentrancy).

If you have any idea on how to improve the snippet and have the time, feel free to post an updated example or (even better) edit the wiki directly, this will greatly benefit newer developers.

I meant something like this:

Code: Select all

// BCI: Add a real d-pointer
typedef QHash<const Foo *, FooPrivate *> FooPrivateHash;
Q_GLOBAL_STATIC(FooPrivateHash, d_func);
Q_GLOBAL_STATIC(QMutex, fooPrivateMutex);

static FooPrivate* d(const Foo* foo )
{   QMutexLocker locker(fooPrivateMutex());
    FooPrivate* ret = d_func()->value( foo, 0 );
    if ( ! ret ) {
        ret = new FooPrivate;
        d_func()->insert( foo, ret );
    }
    return ret;
}

static void delete_d(const Foo* foo )
{   QMutexLocker locker(fooPrivateMutex());
    const FooPrivate* ret = d_func()->value( foo, 0 );
    delete ret;
    d_func()->remove( foo );
}


As I said, I am new to Qt, so I thought I would first post here because I am not completely sure I am doing everything the best way.

The performance of the code above could be optimized a little (at least one could replace the two Q_GLOBAL_STATIC's with one, by defining a new structure) but that would probably not be worth it, since a real d-pointer would be much faster anyway.

(In addition to adding a mutex, I also had to replace the second line in the original code snippet with the second and third line in my code above because otherwise I couldn't even compile the example with the default g++ settings.)

By the way, what does "BCI" stand for? Brain–Computer Interface?

Not sure what BCI means - it could be a typo. In that context I suspect it was related to ABI - Application Binary Interface which is something both Qt and KDE aim to keep to allow applications compiled for older versions to be used without issues on newer versions of Qt/KDE.

If you suspect there may be faults with that article I would recommend sending a email to kde-core-devel@kde.org so the comments can be reviewed and the article adjusted if necessary.