Utils: Allow range-for access to Environment dict

Also refactor functions to use modern c++.

Change-Id: I29aaf92800890048bc1af198cd1726cfe04d573e
Reviewed-by: Cristian Adam <cristian.adam@qt.io>

src/libs/utils/environment.cpp

@@ -60,10 +60,10 @@ EnvironmentItems Environment::diff(const Environment &other, bool checkAppendPre
 Environment::FindResult Environment::find(const QString &name) const
 {
     const NameValueDictionary &dict = resolved();
-    const auto it = dict.constFind(name);
-    if (it == dict.constEnd())
+    const auto it = dict.find(name);
+    if (it == dict.end())
         return {};
-     return Entry{it.key().name, it.value().first, it.value().second};
+    return Entry{it.key(), it.value(), it.enabled()};
 }
 
 void Environment::forEachEntry(const std::function<void(const QString &, const QString &, bool)> &callBack) const
@@ -124,11 +124,10 @@ QStringList Environment::toStringList() const
 
 QProcessEnvironment Environment::toProcessEnvironment() const
 {
-    const NameValueDictionary &dict = resolved();
     QProcessEnvironment result;
-    for (auto it = dict.m_values.constBegin(); it != dict.m_values.constEnd(); ++it) {
-        if (it.value().second)
-            result.insert(it.key().name, expandedValueForKey(dict.key(it)));
+    for (const auto &[key, _, enabled] : resolved()) {
+        if (enabled)
+            result.insert(key, expandedValueForKey(key));
     }
     return result;
 }

src/libs/utils/environmentmodel.cpp

@@ -38,38 +38,29 @@ public:
 
     int findInChanges(const QString &name) const
     {
-        for (int i = 0; i < m_items.size(); ++i)
-            if (m_items.at(i).name.compare(name,
-                                           m_baseNameValueDictionary.nameCaseSensitivity()) == 0) {
+        for (int i = 0; i < m_items.size(); ++i) {
+            if (m_items.at(i).name.compare(name, m_baseNameValueDictionary.nameCaseSensitivity())
+                == 0) {
                 return i;
             }
+        }
         return -1;
     }
 
     int findInResultInsertPosition(const QString &name) const
     {
-        NameValueDictionary::const_iterator it;
-        int i = 0;
-        for (it = m_resultNameValueDictionary.constBegin();
-             it != m_resultNameValueDictionary.constEnd();
-             ++it, ++i)
-            if (it.key() > DictKey(name, m_resultNameValueDictionary.nameCaseSensitivity()))
-                return i;
-        return m_resultNameValueDictionary.size();
+        const auto it = m_resultNameValueDictionary.find(name);
+        if (it == m_resultNameValueDictionary.end())
+            return m_resultNameValueDictionary.size();
+        return std::distance(m_resultNameValueDictionary.begin(), it);
     }
 
     int findInResult(const QString &name) const
     {
-        NameValueDictionary::const_iterator it;
-        int i = 0;
-        for (it = m_resultNameValueDictionary.constBegin();
-             it != m_resultNameValueDictionary.constEnd();
-             ++it, ++i)
-            if (m_resultNameValueDictionary.key(it)
-                    .compare(name, m_resultNameValueDictionary.nameCaseSensitivity()) == 0) {
-                return i;
-            }
-        return -1;
+        const auto it = m_resultNameValueDictionary.find(name);
+        if (it == m_resultNameValueDictionary.end())
+            return -1;
+        return std::distance(m_resultNameValueDictionary.begin(), it);
     }
 
     NameValueDictionary m_baseNameValueDictionary;
@@ -88,8 +79,8 @@ EnvironmentModel::~EnvironmentModel() = default;
 
 QString EnvironmentModel::indexToVariable(const QModelIndex &index) const
 {
-    const auto it = std::next(d->m_resultNameValueDictionary.constBegin(), index.row());
-    return d->m_resultNameValueDictionary.key(it);
+    const auto it = std::next(d->m_resultNameValueDictionary.begin(), index.row());
+    return it.key();
 }
 
 void EnvironmentModel::setBaseEnvironment(const Environment &env)
@@ -133,14 +124,14 @@ QVariant EnvironmentModel::data(const QModelIndex &index, int role) const
     if (!index.isValid())
         return QVariant();
 
-    const auto resultIterator = std::next(d->m_resultNameValueDictionary.constBegin(), index.row());
+    const auto it = std::next(d->m_resultNameValueDictionary.begin(), index.row());
 
     switch (role) {
     case Qt::DisplayRole:
     case Qt::EditRole:
     case Qt::ToolTipRole:
         if (index.column() == 0)
-            return d->m_resultNameValueDictionary.key(resultIterator);
+            return it.key();
         if (index.column() == 1) {
             // Do not return "<UNSET>" when editing a previously unset variable:
             if (role == Qt::EditRole) {
@@ -148,7 +139,7 @@ QVariant EnvironmentModel::data(const QModelIndex &index, int role) const
                 if (pos != -1 && d->m_items.at(pos).operation == EnvironmentItem::Unset)
                     return QString();
             }
-            QString value = d->m_resultNameValueDictionary.value(resultIterator);
+            QString value = it.value();
             if (role == Qt::ToolTipRole && value.length() > 80) {
                 if (currentEntryIsPathList(index)) {
                     // For path lists, display one entry per line without separator
@@ -166,13 +157,12 @@ QVariant EnvironmentModel::data(const QModelIndex &index, int role) const
         break;
     case Qt::FontRole: {
         QFont f;
-        f.setStrikeOut(!d->m_resultNameValueDictionary.isEnabled(resultIterator));
+        f.setStrikeOut(!it.enabled());
         return f;
     }
     case Qt::ForegroundRole: {
         const QPalette p = QGuiApplication::palette();
-        return p.color(changes(d->m_resultNameValueDictionary.key(resultIterator))
-                    ? QPalette::Link : QPalette::Text);
+        return p.color(changes(it.key()) ? QPalette::Link : QPalette::Text);
     }
     }
     return QVariant();
@@ -237,13 +227,11 @@ bool EnvironmentModel::setData(const QModelIndex &index, const QVariant &value,
         // We are changing an existing value:
         const QString stringValue = value.toString();
         if (changesPos != -1) {
-            const auto oldIt = d->m_baseNameValueDictionary.constFind(oldName);
-            const auto newIt = d->m_resultNameValueDictionary.constFind(oldName);
+            const auto oldIt = d->m_baseNameValueDictionary.find(oldName);
+            const auto newIt = d->m_resultNameValueDictionary.find(oldName);
             // We have already changed this value
-            if (oldIt != d->m_baseNameValueDictionary.constEnd()
-                    && stringValue == d->m_baseNameValueDictionary.value(oldIt)
-                    && d->m_baseNameValueDictionary.isEnabled(oldIt)
-                            == d->m_resultNameValueDictionary.isEnabled(newIt)) {
+            if (oldIt != d->m_baseNameValueDictionary.end() && stringValue == oldIt.value()
+                && oldIt.enabled() == newIt.enabled()) {
                 // ... and now went back to the base value
                 d->m_items.removeAt(changesPos);
             } else {
@@ -352,21 +340,19 @@ void EnvironmentModel::unsetVariable(const QString &name)
 void EnvironmentModel::toggleVariable(const QModelIndex &idx)
 {
     const QString name = indexToVariable(idx);
-    const auto newIt = d->m_resultNameValueDictionary.constFind(name);
-    QTC_ASSERT(newIt != d->m_resultNameValueDictionary.constEnd(), return);
-    const auto op = d->m_resultNameValueDictionary.isEnabled(newIt)
-            ? EnvironmentItem::SetDisabled : EnvironmentItem::SetEnabled;
+    const auto newIt = d->m_resultNameValueDictionary.find(name);
+    QTC_ASSERT(newIt != d->m_resultNameValueDictionary.begin(), return);
+    const auto op = newIt.enabled() ? EnvironmentItem::SetDisabled : EnvironmentItem::SetEnabled;
     const int changesPos = d->findInChanges(name);
     if (changesPos != -1) {
-        const auto oldIt = d->m_baseNameValueDictionary.constFind(name);
-        if (oldIt == d->m_baseNameValueDictionary.constEnd()
-                || oldIt.value().first != newIt.value().first) {
+        const auto oldIt = d->m_baseNameValueDictionary.find(name);
+        if (oldIt == d->m_baseNameValueDictionary.end() || oldIt.value() != newIt.value()) {
             d->m_items[changesPos].operation = op;
         } else {
             d->m_items.removeAt(changesPos);
         }
     } else {
-        d->m_items.append({name, d->m_resultNameValueDictionary.value(newIt), op});
+        d->m_items.append({name, newIt.value(), op});
     }
     d->updateResultNameValueDictionary();
     emit dataChanged(index(idx.row(), 0), index(idx.row(), 1));
@@ -381,7 +367,7 @@ bool EnvironmentModel::isUnset(const QString &name)
 
 bool EnvironmentModel::isEnabled(const QString &name) const
 {
-    return d->m_resultNameValueDictionary.isEnabled(d->m_resultNameValueDictionary.constFind(name));
+    return d->m_resultNameValueDictionary.find(name).enabled();
 }
 
 bool EnvironmentModel::canReset(const QString &name)
@@ -413,9 +399,9 @@ void EnvironmentModel::setUserChanges(const EnvironmentItems &items)
         if (d->m_baseNameValueDictionary.osType() == OsTypeWindows) {
             // NameValueDictionary variable names are case-insensitive under windows, but we still
             // want to preserve the case of pre-existing variables.
-            auto it = d->m_baseNameValueDictionary.constFind(name);
-            if (it != d->m_baseNameValueDictionary.constEnd())
-                name = d->m_baseNameValueDictionary.key(it);
+            auto it = d->m_baseNameValueDictionary.find(name);
+            if (it != d->m_baseNameValueDictionary.end())
+                name = it.key();
         }
     }
 

src/libs/utils/namevaluedictionary.cpp

@@ -31,20 +31,12 @@ NameValueDictionary::NameValueDictionary(const NameValuePairs &nameValues)
 
 NameValueMap::iterator NameValueDictionary::findKey(const QString &key)
 {
-    for (auto it = m_values.begin(); it != m_values.end(); ++it) {
-        if (key.compare(it.key().name, nameCaseSensitivity()) == 0)
-            return it;
-    }
-    return m_values.end();
+    return m_values.find(DictKey(key, nameCaseSensitivity()));
 }
 
 NameValueMap::const_iterator NameValueDictionary::findKey(const QString &key) const
 {
-    for (auto it = m_values.constBegin(); it != m_values.constEnd(); ++it) {
-        if (key.compare(it.key().name, nameCaseSensitivity()) == 0)
-            return it;
-    }
-    return m_values.constEnd();
+    return m_values.find(DictKey(key, nameCaseSensitivity()));
 }
 
 QStringList NameValueDictionary::toStringList() const
@@ -87,11 +79,6 @@ QString NameValueDictionary::value(const QString &key) const
     return it != m_values.end() && it.value().second ? it.value().first : QString();
 }
 
-NameValueDictionary::const_iterator NameValueDictionary::constFind(const QString &name) const
-{
-    return findKey(name);
-}
-
 int NameValueDictionary::size() const
 {
     return m_values.size();
@@ -107,24 +94,26 @@ void NameValueDictionary::modify(const EnvironmentItems &items)
 
 EnvironmentItems NameValueDictionary::diff(const NameValueDictionary &other, bool checkAppendPrepend) const
 {
-    NameValueMap::const_iterator thisIt = constBegin();
-    NameValueMap::const_iterator otherIt = other.constBegin();
+    NameValueMap::const_iterator thisIt = m_values.begin();
+    NameValueMap::const_iterator otherIt = other.m_values.begin();
 
     EnvironmentItems result;
-    while (thisIt != constEnd() || otherIt != other.constEnd()) {
-        if (thisIt == constEnd()) {
-            result.append({other.key(otherIt), other.value(otherIt),
-                otherIt.value().second ? EnvironmentItem::SetEnabled : EnvironmentItem::SetDisabled});
+    while (thisIt != m_values.end() || otherIt != other.m_values.end()) {
+        if (thisIt == m_values.end()) {
+            const auto enabled = otherIt.value().second ? EnvironmentItem::SetEnabled
+                                                        : EnvironmentItem::SetDisabled;
+            result.append({otherIt.key().name, otherIt.value().first, enabled});
             ++otherIt;
-        } else if (otherIt == other.constEnd()) {
-            result.append(EnvironmentItem(key(thisIt), QString(), EnvironmentItem::Unset));
+        } else if (otherIt == other.m_values.end()) {
+            result.append(EnvironmentItem(thisIt.key().name, QString(), EnvironmentItem::Unset));
             ++thisIt;
         } else if (thisIt.key() < otherIt.key()) {
-            result.append(EnvironmentItem(key(thisIt), QString(), EnvironmentItem::Unset));
+            result.append(EnvironmentItem(thisIt.key().name, QString(), EnvironmentItem::Unset));
             ++thisIt;
         } else if (thisIt.key() > otherIt.key()) {
-            result.append({other.key(otherIt), otherIt.value().first,
-                otherIt.value().second ? EnvironmentItem::SetEnabled : EnvironmentItem::SetDisabled});
+            const auto enabled = otherIt.value().second ? EnvironmentItem::SetEnabled
+                                                        : EnvironmentItem::SetDisabled;
+            result.append({otherIt.key().name, otherIt.value().first, enabled});
             ++otherIt;
         } else {
             const QString &oldValue = thisIt.value().first;
@@ -137,16 +126,19 @@ EnvironmentItems NameValueDictionary::diff(const NameValueDictionary &other, boo
                     QString appended = newValue.right(newValue.size() - oldValue.size());
                     if (appended.startsWith(OsSpecificAspects::pathListSeparator(osType())))
                         appended.remove(0, 1);
-                    result.append(EnvironmentItem(other.key(otherIt), appended, EnvironmentItem::Append));
+                    result.append(
+                        EnvironmentItem(otherIt.key().name, appended, EnvironmentItem::Append));
                 } else if (checkAppendPrepend && newValue.endsWith(oldValue)
                            && oldEnabled == newEnabled) {
                     QString prepended = newValue.left(newValue.size() - oldValue.size());
                     if (prepended.endsWith(OsSpecificAspects::pathListSeparator(osType())))
                         prepended.chop(1);
-                    result.append(EnvironmentItem(other.key(otherIt), prepended, EnvironmentItem::Prepend));
+                    result.append(
+                        EnvironmentItem(otherIt.key().name, prepended, EnvironmentItem::Prepend));
                 } else {
-                    result.append({other.key(otherIt), newValue, newEnabled
-                            ? EnvironmentItem::SetEnabled : EnvironmentItem::SetDisabled});
+                    const auto enabled = newEnabled ? EnvironmentItem::SetEnabled
+                                                    : EnvironmentItem::SetDisabled;
+                    result.append({otherIt.key().name, newValue, enabled});
                 }
             }
             ++otherIt;
@@ -158,7 +150,7 @@ EnvironmentItems NameValueDictionary::diff(const NameValueDictionary &other, boo
 
 bool NameValueDictionary::hasKey(const QString &key) const
 {
-    return findKey(key) != constEnd();
+    return m_values.find(DictKey(key, nameCaseSensitivity())) != m_values.end();
 }
 
 OsType NameValueDictionary::osType() const

src/libs/utils/namevaluedictionary.h

@@ -36,7 +36,38 @@ using NameValueMap = QMap<DictKey, QPair<QString, bool>>;
 class QTCREATOR_UTILS_EXPORT NameValueDictionary
 {
 public:
-    using const_iterator = NameValueMap::const_iterator;
+    class const_iterator
+    {
+        NameValueMap::const_iterator it;
+
+    public:
+        const_iterator(NameValueMap::const_iterator it)
+            : it(it)
+        {}
+        // clang-format off
+        const_iterator &operator++() { ++it; return *this; }
+        const_iterator &operator++(int) { it++; return *this; }
+        const_iterator &operator--() { --it; return *this; }
+        const_iterator &operator--(int) { it--; return *this; }
+        // clang-format on
+
+        bool operator==(const const_iterator &other) const { return it == other.it; }
+        bool operator!=(const const_iterator &other) const { return it != other.it; }
+        std::tuple<QString, QString, bool> operator*() const
+        {
+            return std::make_tuple(it.key().name, it.value().first, it.value().second);
+        }
+
+        QString key() const { return it.key().name; }
+        QString value() const { return it.value().first; }
+        bool enabled() const { return it.value().second; }
+
+        using difference_type = NameValueMap::const_iterator::difference_type;
+        using value_type = std::tuple<QString, QString, bool>;
+        using pointer = const value_type *;
+        using reference = const value_type &;
+        using iterator_category = NameValueMap::const_iterator::iterator_category;
+    };
 
     explicit NameValueDictionary(OsType osType = HostOsInfo::hostOs())
         : m_osType(osType)
@@ -60,13 +91,9 @@ public:
     void clear();
     int size() const;
 
-    QString key(const_iterator it) const { return it.key().name; }
-    QString value(const_iterator it) const { return it.value().first; }
-    bool isEnabled(const_iterator it) const { return it.value().second; }
-
-    const_iterator constBegin() const { return m_values.constBegin(); }
-    const_iterator constEnd() const { return m_values.constEnd(); }
-    const_iterator constFind(const QString &name) const;
+    const const_iterator begin() const { return const_iterator(m_values.begin()); }
+    const const_iterator end() const { return const_iterator(m_values.end()); }
+    const const_iterator find(const QString &key) const { return const_iterator(findKey(key)); }
 
     friend bool operator!=(const NameValueDictionary &first, const NameValueDictionary &second)
     {
@@ -81,7 +108,7 @@ public:
 protected:
     friend class Environment;
     NameValueMap::iterator findKey(const QString &key);
-    const_iterator findKey(const QString &key) const;
+    NameValueMap::const_iterator findKey(const QString &key) const;
 
     NameValueMap m_values;
     OsType m_osType;

src/libs/utils/namevalueitem.cpp

@@ -112,9 +112,9 @@ static QString expand(const NameValueDictionary *dictionary, QString value)
                     end = value.indexOf('}', i);
                 if (end != -1) {
                     const QString &key = value.mid(i + 2, end - i - 2);
-                    NameValueDictionary::const_iterator it = dictionary->constFind(key);
-                    if (it != dictionary->constEnd())
-                        value.replace(i, end - i + 1, it.value().first);
+                    const NameValueDictionary::const_iterator it = dictionary->find(key);
+                    if (it != dictionary->end())
+                        value.replace(i, end - i + 1, it.value());
                     ++replaceCount;
                     QTC_ASSERT(replaceCount < 100, break);
                 }
@@ -137,9 +137,9 @@ void EnvironmentItem::apply(NameValueDictionary *dictionary, Operation op) const
         dictionary->unset(name);
         break;
     case Prepend: {
-        const NameValueDictionary::const_iterator it = dictionary->constFind(name);
-        if (it != dictionary->constEnd()) {
-            QString v = dictionary->value(it);
+        const NameValueDictionary::const_iterator it = dictionary->find(name);
+        if (it != dictionary->end()) {
+            QString v = it.value();
             const QChar pathSep = HostOsInfo::pathListSeparator();
             int sepCount = 0;
             if (v.startsWith(pathSep))
@@ -157,9 +157,9 @@ void EnvironmentItem::apply(NameValueDictionary *dictionary, Operation op) const
         }
     } break;
     case Append: {
-        const NameValueDictionary::const_iterator it = dictionary->constFind(name);
-        if (it != dictionary->constEnd()) {
-            QString v = dictionary->value(it);
+        const NameValueDictionary::const_iterator it = dictionary->find(name);
+        if (it != dictionary->end()) {
+            QString v = it.value();
             const QChar pathSep = HostOsInfo::pathListSeparator();
             int sepCount = 0;
             if (v.endsWith(pathSep))

src/plugins/cmakeprojectmanager/presetsmacros.cpp

@@ -136,7 +136,7 @@ void expand(const PresetType &preset, Environment &env, const FilePath &sourceDi
 
     const Environment combinedEnv = getEnvCombined(preset.environment, env);
     const Environment parentEnv = env;
-    preset.environment->forEachEntry([&](const QString &key, QString value, bool enabled) {
+    for (auto [key, value, enabled] : preset.environment->resolved()) {
         if (!enabled)
             return;
         expandAllButEnv(preset, sourceDirectory, value);
@@ -152,7 +152,7 @@ void expand(const PresetType &preset, Environment &env, const FilePath &sourceDi
         expandAllButEnv(preset, sourceDirectory, value);
 
         env.set(key, value);
-    });
+    }
 }
 
 template<class PresetType>
@@ -161,28 +161,25 @@ void expand(const PresetType &preset, EnvironmentItems &envItems, const FilePath
     if (!preset.environment)
         return;
 
-    preset.environment->forEachEntry(
-        [&preset,
-         &sourceDirectory,
-         &envItems](const QString &key, QString value, bool enabled) {
-            if (!enabled)
-                return;
-            expandAllButEnv(preset, sourceDirectory, value);
-            value = expandMacroEnv("env", value, [&preset](const QString &macroName) {
-                if (preset.environment->hasKey(macroName))
-                    return preset.environment->value(macroName);
-                return QString("${%1}").arg(macroName);
-            });
-
-            value = expandMacroEnv("penv", value, [](const QString &macroName) {
-                return QString("${%1}").arg(macroName);
-            });
-
-            // Make sure to expand the CMake macros also for environment variables
-            expandAllButEnv(preset, sourceDirectory, value);
-
-            envItems.emplace_back(Utils::EnvironmentItem(key, value));
+    for (auto [key, value, enabled] : preset.environment->resolved()) {
+        if (!enabled)
+            return;
+        expandAllButEnv(preset, sourceDirectory, value);
+        value = expandMacroEnv("env", value, [&preset](const QString &macroName) {
+            if (preset.environment->hasKey(macroName))
+                return preset.environment->value(macroName);
+            return QString("${%1}").arg(macroName);
         });
+
+        value = expandMacroEnv("penv", value, [](const QString &macroName) {
+            return QString("${%1}").arg(macroName);
+        });
+
+        // Make sure to expand the CMake macros also for environment variables
+        expandAllButEnv(preset, sourceDirectory, value);
+
+        envItems.emplace_back(Utils::EnvironmentItem(key, value));
+    }
 }
 
 template<class PresetType>