QProcessEnvironment: simplify locking

The mutex is only protecting 'nameMap'. Proof: it's only defined on
platforms on which there is a 'nameMap'. Also, nothing else is
mutable, so no lazy init going on here.

So, drop all the mutex protection, except where we access 'nameMap',
and draw the mutex as close as possible to the nameMap uses, iow: copy
ctor, prepareName() and nameToString().

As a consequence, the old (Ordered)MutexLocker class only needs to be
defined on Unix.

Change-Id: Ic969313bc48ad7ebf24c5dca7fd48359956b048d
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

3 files changed, 22 insertions, 37 deletions

diff --git a/src/corelib/io/qprocess.cpp b/src/corelib/io/qprocess.cpp
index 0b964e6a21..35ca2542f7 100644
--- a/src/corelib/io/qprocess.cpp
+++ b/src/corelib/io/qprocess.cpp
@@ -202,6 +202,7 @@ void QProcessEnvironmentPrivate::insert(const QProcessEnvironmentPrivate &other)
         vars.insert(it.key(), it.value());
 
 #ifdef Q_OS_UNIX
+    const OrderedNameMapMutexLocker locker(this, &other);
     auto nit = other.nameMap.constBegin();
     const auto nend = other.nameMap.constEnd();
     for ( ; nit != nend; ++nit)
@@ -275,7 +276,6 @@ bool QProcessEnvironment::operator==(const QProcessEnvironment &other) const
         return true;
     if (d) {
         if (other.d) {
-            QProcessEnvironmentPrivate::OrderedMutexLocker locker(d, other.d);
             return d->vars == other.d->vars;
         } else {
             return isEmpty();
@@ -322,7 +322,6 @@ bool QProcessEnvironment::contains(const QString &name) const
 {
     if (!d)
         return false;
-    QProcessEnvironmentPrivate::MutexLocker locker(d);
     return d->vars.contains(d->prepareName(name));
 }
 
@@ -373,7 +372,6 @@ QString QProcessEnvironment::value(const QString &name, const QString &defaultVa
     if (!d)
         return defaultValue;
 
-    QProcessEnvironmentPrivate::MutexLocker locker(d);
     const auto it = d->vars.constFind(d->prepareName(name));
     if (it == d->vars.constEnd())
         return defaultValue;
@@ -398,7 +396,6 @@ QStringList QProcessEnvironment::toStringList() const
 {
     if (!d)
         return QStringList();
-    QProcessEnvironmentPrivate::MutexLocker locker(d);
     return d->toList();
 }
 
@@ -412,7 +409,6 @@ QStringList QProcessEnvironment::keys() const
 {
     if (!d)
         return QStringList();
-    QProcessEnvironmentPrivate::MutexLocker locker(d);
     return d->keys();
 }
 
@@ -429,7 +425,6 @@ void QProcessEnvironment::insert(const QProcessEnvironment &e)
         return;
 
     // our re-impl of detach() detaches from null
-    QProcessEnvironmentPrivate::MutexLocker locker(e.d);
     d->insert(*e.d);
 }
 
diff --git a/src/corelib/io/qprocess_p.h b/src/corelib/io/qprocess_p.h
index d02e87837c..2587530c09 100644
--- a/src/corelib/io/qprocess_p.h
+++ b/src/corelib/io/qprocess_p.h
@@ -146,16 +146,22 @@ public:
     inline QString nameToString(const Key &name) const { return name; }
     inline Value prepareValue(const QString &value) const { return value; }
     inline QString valueToString(const Value &value) const { return value; }
-    struct MutexLocker {
-        MutexLocker(const QProcessEnvironmentPrivate *) {}
+#else
+    struct NameMapMutexLocker : public QMutexLocker
+    {
+        NameMapMutexLocker(const QProcessEnvironmentPrivate *d) : QMutexLocker(&d->nameMapMutex) {}
     };
-    struct OrderedMutexLocker {
-        OrderedMutexLocker(const QProcessEnvironmentPrivate *,
-                           const QProcessEnvironmentPrivate *) {}
+    struct OrderedNameMapMutexLocker : public QOrderedMutexLocker
+    {
+        OrderedNameMapMutexLocker(const QProcessEnvironmentPrivate *d1,
+                                  const QProcessEnvironmentPrivate *d2)
+            : QOrderedMutexLocker(&d1->nameMapMutex, &d2->nameMapMutex)
+        {}
     };
-#else
+
     inline Key prepareName(const QString &name) const
     {
+        const NameMapMutexLocker locker(this);
         Key &ent = nameMap[name];
         if (ent.isEmpty())
             ent = name.toLocal8Bit();
@@ -164,40 +170,27 @@ public:
     inline QString nameToString(const Key &name) const
     {
         const QString sname = QString::fromLocal8Bit(name);
-        nameMap[sname] = name;
+        {
+            const NameMapMutexLocker locker(this);
+            nameMap[sname] = name;
+        }
         return sname;
     }
     inline Value prepareValue(const QString &value) const { return Value(value); }
     inline QString valueToString(const Value &value) const { return value.string(); }
 
-    struct MutexLocker : public QMutexLocker
-    {
-        MutexLocker(const QProcessEnvironmentPrivate *d) : QMutexLocker(&d->mutex) {}
-    };
-    struct OrderedMutexLocker : public QOrderedMutexLocker
-    {
-        OrderedMutexLocker(const QProcessEnvironmentPrivate *d1,
-                           const QProcessEnvironmentPrivate *d2) :
-            QOrderedMutexLocker(&d1->mutex, &d2->mutex)
-        {}
-    };
-
     QProcessEnvironmentPrivate() : QSharedData() {}
     QProcessEnvironmentPrivate(const QProcessEnvironmentPrivate &other) :
-        QSharedData()
+        QSharedData(), vars(other.vars)
     {
-        // This being locked ensures that the functions that only assign
-        // d pointers don't need explicit locking.
         // We don't need to lock our own mutex, as this object is new and
         // consequently not shared. For the same reason, non-const methods
         // do not need a lock, as they detach objects (however, we need to
         // ensure that they really detach before using prepareName()).
-        MutexLocker locker(&other);
-        vars = other.vars;
+        NameMapMutexLocker locker(&other);
         nameMap = other.nameMap;
-        // We need to detach our members, so that our mutex can protect them.
-        // As we are being detached, they likely would be detached a moment later anyway.
-        vars.detach();
+        // We need to detach our nameMap, so that our mutex can protect it.
+        // As we are being detached, it likely would be detached a moment later anyway.
         nameMap.detach();
     }
 #endif
@@ -208,8 +201,7 @@ public:
 #ifdef Q_OS_UNIX
     typedef QHash<QString, Key> NameHash;
     mutable NameHash nameMap;
-
-    mutable QMutex mutex;
+    mutable QMutex nameMapMutex;
 #endif
 
     static QProcessEnvironment fromList(const QStringList &list);
diff --git a/src/corelib/io/qprocess_unix.cpp b/src/corelib/io/qprocess_unix.cpp
index 1d5b76a8a4..951fc4ccaa 100644
--- a/src/corelib/io/qprocess_unix.cpp
+++ b/src/corelib/io/qprocess_unix.cpp
@@ -438,7 +438,6 @@ void QProcessPrivate::startProcess()
     int envc = 0;
     char **envp = 0;
     if (environment.d.constData()) {
-        QProcessEnvironmentPrivate::MutexLocker locker(environment.d);
         envp = _q_dupEnvironment(environment.d.constData()->vars, &envc);
     }
 
@@ -970,7 +969,6 @@ bool QProcessPrivate::startDetached(qint64 *pid)
             int envc = 0;
             char **envp = nullptr;
             if (environment.d.constData()) {
-                QProcessEnvironmentPrivate::MutexLocker locker(environment.d);
                 envp = _q_dupEnvironment(environment.d.constData()->vars, &envc);
             }