Fixed clazy warnings: unnecessary containers and containers being detached in range-for loops.

This commit is contained in:
Mat Sutcliffe
2018-12-17 17:42:44 +00:00
parent 684ffeb671
commit 462172a87f
18 changed files with 114 additions and 139 deletions

View File

@@ -49,9 +49,8 @@ namespace BlackMisc
CDirectoryUtils::currentApplicationDataDirectoryMapWithoutCurrentVersion() :
CDirectoryUtils::applicationDataDirectoryMapWithoutCurrentVersion();
for (const QString &directory : otherVersions.keys())
for (const CApplicationInfo &info : otherVersions)
{
CApplicationInfo info(otherVersions.value(directory));
this->push_back(info);
}
return this->size();

View File

@@ -304,35 +304,21 @@ namespace BlackMisc
QMap<QString, int> count;
for (const CAircraftIcaoCode &icao : *this)
{
if (!icao.hasManufacturer()) continue;
const QString m(icao.getManufacturer());
if (count.contains(m))
{
count[m]++;
}
else
{
count[m] = 1;
}
if (!icao.hasManufacturer()) { continue; }
count[icao.getManufacturer()]++;
}
return count;
}
QPair<QString, int> CAircraftIcaoCodeList::maxCountManufacturer() const
{
if (this->isEmpty()) return QPair<QString, int>("", 0);
const QMap<QString, int> counts(countManufacturers());
QPair<QString, int> max;
for (const QString &m : counts.keys())
if (counts.isEmpty()) return { {}, 0 };
const auto pair = *std::max_element(counts.keyValueBegin(), counts.keyValueEnd(), [](const auto &a, const auto &b)
{
const int mv = counts[m];
if (mv > max.second)
{
max.first = m;
max.second = mv;
}
}
return max;
return a.second < b.second;
});
return { pair.first, pair.second };
}
CAircraftIcaoCodeList CAircraftIcaoCodeList::fromDatabaseJson(const QJsonArray &array, bool ignoreIncompleteAndDuplicates, CAircraftIcaoCodeList *inconsistent)

View File

@@ -423,7 +423,7 @@ namespace BlackMisc
if (updateUuid) { m_admittedQueue.clear(); }
else if (! m_admittedQueue.isEmpty()) { m_admittedQueue.intersect(QSet<QString>::fromList(m_timestamps.keys())); }
for (const auto &key : m_timestamps.keys())
for (const auto &key : m_timestamps.keys()) // clazy:exclude=container-anti-pattern,range-loop
{
if (deferrals.contains(key) && ! m_admittedValues.contains(key)) { m_timestamps.remove(key); }
}

View File

@@ -318,6 +318,26 @@ namespace BlackMisc
//! Returns const iterator at the end of the dictionary
const_iterator cend() const { return m_impl.cend(); }
//! Returns const iterator for iterating over keys
//! @{
auto keyBegin() const { return m_impl.keyBegin(); }
auto keyEnd() const { return m_impl.keyEnd(); }
//! @}
//! Returns iterator for iterating over keys and values together
//! @{
auto keyValueBegin() { return m_impl.keyValueBegin(); }
auto keyValueEnd() { return m_impl.keyValueEnd(); }
//! @}
//! Returns const iterator for iterating over keys and values together
//! @{
auto keyValueBegin() const { return m_impl.keyValueBegin(); }
auto constKeyValueBegin() const { return m_impl.constKeyValueBegin(); }
auto keyValueEnd() const { return m_impl.keyValueEnd(); }
auto constKeyValueEnd() const { return m_impl.constKeyValueEnd(); }
//! @}
//! Removes all items from the dictionary
void clear() { m_impl.clear(); }
@@ -369,8 +389,8 @@ namespace BlackMisc
//! Return key assigned to value or if key is not found defaultKey
const Key key(const Value &value, const Key & defaultKey) const { return m_impl.key(value, defaultKey); }
//! Return a range of all keys
CRange<Iterators::KeyIterator<const_iterator>> keys() const { return makeRange(Iterators::makeKeyIterator(begin()), end()); }
//! Return a range of all keys (does not allocate a temporary container)
auto keys() const { return makeRange(keyBegin(), keyEnd()); }
//! Remove all items with key from the dictionary
int remove(const Key &key) { return m_impl.remove(key); }
@@ -387,7 +407,7 @@ namespace BlackMisc
//! Returns the value associated with the key or if key is not found defaultValue
const Value value(const Key &key, const Value &defaultValue) const { return m_impl.value(key, defaultValue); }
//! Return a range of all values
//! Return a range of all values (does not allocate a temporary container)
CRange<const_iterator> values() const { return makeRange(begin(), end()); }
//! Copy assignment.
@@ -407,7 +427,6 @@ namespace BlackMisc
Value &operator [](const Key &key) { return m_impl[key]; }
//! Access an element by its key.
//! \note If dictionary does not contain any item with key, a default constructed value will be inserted.
const Value operator [](const Key &key) const { return m_impl[key]; }
//! Test for equality.

View File

@@ -289,7 +289,7 @@ namespace BlackMisc
QString last;
QStringList result;
for (const QString &path : dirs)
for (const QString &path : as_const(dirs))
{
if (path.isEmpty()) { continue; }
if (last.isEmpty() || !path.startsWith(last, cs))

View File

@@ -95,54 +95,6 @@ namespace BlackMisc
return Private::makeInsertIterator(container, THasPushBack<T>());
}
/*!
* Iterator wrapper for Qt's STL-style associative container iterators, when dereferenced return the key instead of the value.
*
* By creating a CRange from such iterators, it is possible to create a container of keys without copying them.
*/
template <class I> class KeyIterator
: public std::iterator<std::bidirectional_iterator_tag, std::decay_t<decltype(std::declval<I>().key())>>
{
public:
//! Constructor
KeyIterator(I iterator) : m_iterator(iterator) {}
//! Advance to the next element.
//! Undefined if iterator is at the end.
//! @{
KeyIterator &operator ++() { ++m_iterator; return *this; }
KeyIterator operator ++(int) { auto copy = *this; ++m_iterator; return copy; }
//! @}
//! Regress to the previous element.
//! Undefined if iterator is at the beginning.
//! @{
KeyIterator &operator --() { --m_iterator; return *this; }
KeyIterator operator --(int) { auto copy = *this; --m_iterator; return copy; }
//! @}
//! Return the value at this iterator position.
auto value() const { return m_iterator.value(); }
//! Return the key at this iterator position.
//! @{
auto key() const { return m_iterator.key(); }
auto operator *() const { return key(); }
//! @}
//! Indirection operator: pointer to the key at this iterator position.
auto operator ->() const { return &key(); }
//! Equality operators.
//! @{
bool operator ==(const KeyIterator &other) const { return m_iterator == other.m_iterator; }
bool operator !=(const KeyIterator &other) const { return m_iterator != other.m_iterator; }
//! @}
private:
I m_iterator;
};
/*!
* Iterator wrapper which applies some transformation function to each element.
*
@@ -325,14 +277,6 @@ namespace BlackMisc
QVector<I> m_iterators;
};
/*!
* Construct a KeyIterator of the appropriate type from deduced template function argument.
*/
template <class I> auto makeKeyIterator(I iterator) -> KeyIterator<I>
{
return { iterator };
}
/*!
* Construct a TransformIterator of the appropriate type from deduced template function arguments.
*/

View File

@@ -321,14 +321,27 @@ namespace BlackMisc
}
/*!
* Returns a const CRange for iterating over the keys of an associative container.
* Returns a const CRange for iterating over the keys of a Qt associative container.
*
* This is more efficient than the keys() method of the container, as it doesn't allocate memory.
*/
template <class T>
auto makeKeysRange(const T &container)
{
return makeRange(Iterators::makeKeyIterator(container.cbegin()), container.cend());
return makeRange(container.keyBegin(), container.keyEnd());
}
/*!
* Returns a const CRange for iterating over the keys and values of a Qt associative container.
* The value_type of the returned range is std::pair<T::key_type &, T::value_type &>.
*
* This is more efficient than using the keys() method of the container and thereafter looking up each key,
* as it neither allocates memory, nor performs any key lookups.
*/
template <class T>
auto makePairsRange(const T &container)
{
return makeRange(container.keyValueBegin(), container.keyValueEnd());
}
/*!
@@ -345,6 +358,20 @@ namespace BlackMisc
template <class T>
void makeKeysRange(const T &&container) = delete;
/*!
* Pairs range for a non-const lvalue would be unsafe due to iterator invalidation on detach().
*
* \see http://doc.qt.io/qt-5/containers.html#implicit-sharing-iterator-problem
*/
template <class T>
void makePairsRange(T &container) = delete;
/*!
* Pairs range for a temporary would be unsafe.
*/
template <class T>
void makePairsRange(const T &&container) = delete;
/*
* Member functions of CRangeBase template defined out of line, because they depend on CRange etc.
*/

View File

@@ -352,11 +352,11 @@ namespace BlackMisc
{
const QMap<QString, int> modelStrings = this->countPerModelString();
CAircraftModelList duplicates;
for (const QString &ms : modelStrings.keys())
for (const auto &pair : makePairsRange(modelStrings))
{
if (modelStrings[ms] > 1)
if (pair.second > 1)
{
duplicates.push_back(this->findByModelString(ms, Qt::CaseInsensitive));
duplicates.push_back(this->findByModelString(pair.first, Qt::CaseInsensitive));
}
}
return duplicates;

View File

@@ -62,9 +62,9 @@ namespace BlackMisc
{
const SetupsPerCallsign setups = this->getSetupsPerCallsign();
CCallsignSet callsigns;
for (const CCallsign &cs : setups.keys())
for (const auto &pair : makePairsRange(setups))
{
if (setups.value(cs).logInterpolation()) { callsigns.insert(cs); }
if (pair.second.logInterpolation()) { callsigns.insert(pair.first); }
}
return callsigns;
}
@@ -146,19 +146,19 @@ namespace BlackMisc
void IInterpolationSetupProvider::clearInterpolationLogCallsigns()
{
SetupsPerCallsign setupsCopy = this->getSetupsPerCallsign();
const SetupsPerCallsign setupsCopy = this->getSetupsPerCallsign();
if (setupsCopy.isEmpty()) { return; }
// potential risk, changes inbetween in another thread are missed now
// on the other side, we keep locks for a minimal time frame
SetupsPerCallsign setupsToKeep;
CInterpolationAndRenderingSetupGlobal global = this->getInterpolationSetupGlobal();
for (const CCallsign &cs : setupsCopy.keys())
for (const auto &pair : makePairsRange(setupsCopy))
{
CInterpolationAndRenderingSetupPerCallsign setup = setupsCopy.value(cs);
CInterpolationAndRenderingSetupPerCallsign setup = pair.second;
setup.setLogInterpolation(false);
if (setup.isEqualToGlobal(global)) { continue; }
setupsToKeep.insert(cs, setup);
setupsToKeep.insert(pair.first, setup);
}
{
QWriteLocker l(&m_lockSetup);
@@ -184,9 +184,9 @@ namespace BlackMisc
{
const SetupsPerCallsign setupsCopy = this->getSetupsPerCallsign();
if (setupsCopy.isEmpty()) { return false; }
for (const CCallsign &cs : setupsCopy.keys())
for (const CInterpolationAndRenderingSetupPerCallsign &setup : setupsCopy)
{
if (setupsCopy.value(cs).logInterpolation()) { return true; }
if (setup.logInterpolation()) { return true; }
}
return false;
}