Commit 834fb13b authored by Igor Kushnir's avatar Igor Kushnir Committed by Milian Wolff
Browse files

Remove incorrect move operations for indexed items

Declare copy operations of the affected classes noexcept to improve
performance of classes that contain them.

The implementations of the removed move constructors were wrong in case
either the source or the destination (but not both) were
disk-reference-counted. The move assignment operators were apparently
correct but, as stated in added @note comments and detailed in this
commit message below, they weren't useful in practice.

Note that the removed IndexedInstantiationInformation's move operations
contained another mistake: they used
standardInstantiationInformationIndex() in place of 0 (see the
implementation of IndexedInstantiationInformation(uint index)).

I have analyzed the calls of copy and move constructors, assignment
operators and swap() specializations for the three classes
(IndexedIdentifier, IndexedQualifiedIdentifier,
IndexedInstantiationInformation) by running statistics-printing-patched
KDevelop several times with kdevelop, kate, ktexteditor and
syntax-highlighting projects loaded. The results:
1. In the move assignment operators almost always either the indices are
   equal or neither operand is DUChain-reference-counted. Only in less
   than 0.5% of calls to IndexedQualifiedIdentifier's move assignment
   operator the indices differ and the left operand is
   reference-counted. The right operand is never reference-counted when
   the indices differ.
2. In the move constructors neither operand is ever reference-counted.
3. No swap() specialization is ever called.
4. Even in the copy special operations, never once both operands were
   reference-counted at the same time when the indices differed (the
   same is true for IndexedString, by the way).

So in practice ItemRepositoryReferenceCounting's inc(), dec() and
setIndex() implement the move operations optimally => remove the more
complex moveIndex() helper function.

a17d3f65 similarly removed
IndexedString's move operations.
parent 50de4e84
......@@ -1304,6 +1304,14 @@ IndexedTypeIdentifier::IndexedTypeIdentifier(const QString& identifier, bool isE
, m_pointerConstMask(0)
{ }
// NOTE: the definitions of ItemRepositoryReferenceCounting's inc(), dec() and setIndex() are so
// complex that they can throw exceptions for many reasons. Yet some special member functions of
// Indexed[Qualified]Identifier, which call them, are implicitly (the destructors) or explicitly
// noexcept. The noexcept-ness of these functions is important for correctness and performance.
// This is safe at the moment, because the entire KDevPlatformLanguage library, that contains these
// classes, is compiled with exceptions disabled (-fno-exceptions), which already prevents exception
// propagation to a caller of any non-inline function in this library.
IndexedIdentifier::IndexedIdentifier(unsigned int index)
: m_index(index)
{
......@@ -1320,17 +1328,11 @@ IndexedIdentifier::IndexedIdentifier(const Identifier& id)
{
}
IndexedIdentifier::IndexedIdentifier(const IndexedIdentifier& rhs)
IndexedIdentifier::IndexedIdentifier(const IndexedIdentifier& rhs) noexcept
: IndexedIdentifier(rhs.m_index)
{
}
IndexedIdentifier::IndexedIdentifier(IndexedIdentifier&& rhs) Q_DECL_NOEXCEPT
: m_index(rhs.m_index)
{
rhs.m_index = emptyConstantIdentifierPrivateIndex();
}
IndexedIdentifier::~IndexedIdentifier()
{
ItemRepositoryReferenceCounting::dec(this);
......@@ -1347,13 +1349,7 @@ IndexedIdentifier& IndexedIdentifier::operator=(const Identifier& id)
return operator=(id.index());
}
IndexedIdentifier& IndexedIdentifier::operator=(IndexedIdentifier&& rhs) Q_DECL_NOEXCEPT
{
ItemRepositoryReferenceCounting::moveIndex(this, m_index, &rhs, rhs.m_index, emptyConstantIdentifierPrivateIndex());
return *this;
}
IndexedIdentifier& IndexedIdentifier::operator=(const IndexedIdentifier& id)
IndexedIdentifier& IndexedIdentifier::operator=(const IndexedIdentifier& id) noexcept
{
return operator=(id.m_index);
}
......@@ -1423,17 +1419,11 @@ IndexedQualifiedIdentifier::IndexedQualifiedIdentifier(const QualifiedIdentifier
{
}
IndexedQualifiedIdentifier::IndexedQualifiedIdentifier(const IndexedQualifiedIdentifier& id)
IndexedQualifiedIdentifier::IndexedQualifiedIdentifier(const IndexedQualifiedIdentifier& id) noexcept
: IndexedQualifiedIdentifier(id.m_index)
{
}
IndexedQualifiedIdentifier::IndexedQualifiedIdentifier(IndexedQualifiedIdentifier&& rhs) Q_DECL_NOEXCEPT
: m_index(rhs.m_index)
{
rhs.m_index = emptyConstantQualifiedIdentifierPrivateIndex();
}
IndexedQualifiedIdentifier& IndexedQualifiedIdentifier::operator=(unsigned int index)
{
ifDebug(qCDebug(LANGUAGE) << "(" << ++cnt << ") Assigning to" << identifier().toString() << m_index);
......@@ -1447,17 +1437,11 @@ IndexedQualifiedIdentifier& IndexedQualifiedIdentifier::operator=(const Qualifie
return operator=(id.index());
}
IndexedQualifiedIdentifier& IndexedQualifiedIdentifier::operator=(const IndexedQualifiedIdentifier& rhs)
IndexedQualifiedIdentifier& IndexedQualifiedIdentifier::operator=(const IndexedQualifiedIdentifier& rhs) noexcept
{
return operator=(rhs.m_index);
}
IndexedQualifiedIdentifier& IndexedQualifiedIdentifier::operator=(IndexedQualifiedIdentifier&& rhs) Q_DECL_NOEXCEPT
{
ItemRepositoryReferenceCounting::moveIndex(this, m_index, &rhs, rhs.m_index, emptyConstantQualifiedIdentifierPrivateIndex());
return *this;
}
IndexedQualifiedIdentifier::~IndexedQualifiedIdentifier()
{
ifDebug(qCDebug(LANGUAGE) << "(" << ++cnt << ") Destroying" << identifier().toString() << m_index);
......
......@@ -30,6 +30,13 @@ template <bool>
class IdentifierPrivate;
class IndexedString;
/**
* @note Move constructor and move assignment operator are deliberately not implemented for
* Indexed[Qualified]Identifier. The move operations are tricky to implement correctly and more
* efficiently in practice than the copy operations. swap() could be specialized for these two
* classes, but it would never be called in practice. See a similar note for class IndexedString.
*/
/**
* A helper-class to store an identifier by index in a type-safe way.
*
......@@ -46,11 +53,9 @@ class KDEVPLATFORMLANGUAGE_EXPORT IndexedIdentifier
public:
IndexedIdentifier();
explicit IndexedIdentifier(const Identifier& id);
IndexedIdentifier(const IndexedIdentifier& rhs);
IndexedIdentifier(IndexedIdentifier&& rhs) Q_DECL_NOEXCEPT;
IndexedIdentifier(const IndexedIdentifier& rhs) noexcept;
IndexedIdentifier& operator=(const Identifier& id);
IndexedIdentifier& operator=(const IndexedIdentifier& rhs);
IndexedIdentifier& operator=(IndexedIdentifier&& rhs) Q_DECL_NOEXCEPT;
IndexedIdentifier& operator=(const IndexedIdentifier& rhs) noexcept;
~IndexedIdentifier();
bool operator==(const IndexedIdentifier& rhs) const;
bool operator!=(const IndexedIdentifier& rhs) const;
......@@ -89,11 +94,9 @@ class KDEVPLATFORMLANGUAGE_EXPORT IndexedQualifiedIdentifier
public:
IndexedQualifiedIdentifier();
IndexedQualifiedIdentifier(const QualifiedIdentifier& id);
IndexedQualifiedIdentifier(const IndexedQualifiedIdentifier& rhs);
IndexedQualifiedIdentifier(IndexedQualifiedIdentifier&& rhs) Q_DECL_NOEXCEPT;
IndexedQualifiedIdentifier(const IndexedQualifiedIdentifier& rhs) noexcept;
IndexedQualifiedIdentifier& operator=(const QualifiedIdentifier& id);
IndexedQualifiedIdentifier& operator=(const IndexedQualifiedIdentifier& id);
IndexedQualifiedIdentifier& operator=(IndexedQualifiedIdentifier&& rhs) Q_DECL_NOEXCEPT;
IndexedQualifiedIdentifier& operator=(const IndexedQualifiedIdentifier& id) noexcept;
~IndexedQualifiedIdentifier();
bool operator==(const IndexedQualifiedIdentifier& rhs) const;
bool operator==(const QualifiedIdentifier& id) const;
......
......@@ -158,31 +158,26 @@ IndexedInstantiationInformation::IndexedInstantiationInformation(uint index) : m
ItemRepositoryReferenceCounting::inc(this);
}
IndexedInstantiationInformation::IndexedInstantiationInformation(const IndexedInstantiationInformation& rhs) : m_index(
rhs.m_index)
{
ItemRepositoryReferenceCounting::inc(this);
}
IndexedInstantiationInformation::IndexedInstantiationInformation(IndexedInstantiationInformation&& rhs) noexcept
// NOTE: the definitions of ItemRepositoryReferenceCounting's inc(), dec() and setIndex() are so
// complex that they can throw exceptions for many reasons. Yet some special member functions of
// IndexedInstantiationInformation, which call them, are implicitly (the destructor) or explicitly
// noexcept. The noexcept-ness of these functions is important for correctness and performance.
// This is safe at the moment, because the entire KDevPlatformLanguage library, that contains
// IndexedInstantiationInformation, is compiled with exceptions disabled (-fno-exceptions), which
// already prevents exception propagation to a caller of any non-inline function in this library.
IndexedInstantiationInformation::IndexedInstantiationInformation(const IndexedInstantiationInformation& rhs) noexcept
: m_index(rhs.m_index)
{
rhs.m_index = standardInstantiationInformationIndex();
ItemRepositoryReferenceCounting::inc(this);
}
IndexedInstantiationInformation& IndexedInstantiationInformation::operator=(const IndexedInstantiationInformation& rhs)
IndexedInstantiationInformation& IndexedInstantiationInformation::operator=(const IndexedInstantiationInformation& rhs) noexcept
{
ItemRepositoryReferenceCounting::setIndex(this, m_index, rhs.m_index);
return *this;
}
IndexedInstantiationInformation&
IndexedInstantiationInformation::operator=(IndexedInstantiationInformation&& rhs) noexcept
{
ItemRepositoryReferenceCounting::moveIndex(this, m_index, &rhs, rhs.m_index, standardInstantiationInformationIndex());
return *this;
}
IndexedInstantiationInformation::~IndexedInstantiationInformation()
{
ItemRepositoryReferenceCounting::dec(this);
......
......@@ -21,16 +21,20 @@ class QualifiedIdentifier;
KDEVPLATFORMLANGUAGE_EXPORT DECLARE_LIST_MEMBER_HASH(InstantiationInformation, templateParameters, IndexedType)
/**
* @note Move constructor and move assignment operator are deliberately not implemented for
* IndexedInstantiationInformation. The move operations are tricky to implement correctly and more
* efficiently in practice than the copy operations. swap() could be specialized for this class,
* but it would never be called in practice. See a similar note for class IndexedString.
*/
class KDEVPLATFORMLANGUAGE_EXPORT IndexedInstantiationInformation
: public ReferenceCountManager
{
public:
IndexedInstantiationInformation() noexcept = default;
explicit IndexedInstantiationInformation(uint index);
IndexedInstantiationInformation(const IndexedInstantiationInformation& rhs);
IndexedInstantiationInformation(IndexedInstantiationInformation&& rhs) noexcept;
IndexedInstantiationInformation& operator=(const IndexedInstantiationInformation& rhs);
IndexedInstantiationInformation& operator=(IndexedInstantiationInformation&& rhs) noexcept;
IndexedInstantiationInformation(const IndexedInstantiationInformation& rhs) noexcept;
IndexedInstantiationInformation& operator=(const IndexedInstantiationInformation& rhs) noexcept;
~IndexedInstantiationInformation();
const InstantiationInformation& information() const;
......
......@@ -81,7 +81,6 @@ void TestIdentifier::testIdentifier()
QCOMPARE(moved, copy);
IndexedIdentifier movedIndexed = std::move(indexedId);
QVERIFY(indexedId.isEmpty());
QCOMPARE(movedIndexed, indexedCopy);
}
......@@ -153,7 +152,6 @@ void TestIdentifier::testQualifiedIdentifier()
QCOMPARE(moved, copy);
IndexedQualifiedIdentifier movedIndexed = std::move(indexedId);
QVERIFY(indexedId.isEmpty());
QCOMPARE(movedIndexed, indexedCopy);
copy.clear();
......
......@@ -66,39 +66,6 @@ struct ItemRepositoryReferenceCounting {
m_index = index;
}
}
template <typename Item>
static void moveIndex(Item* lhs, unsigned int& lhs_index, Item* rhs, unsigned int& rhs_index,
unsigned int emptyIndex)
{
Q_ASSERT(lhs);
Q_ASSERT(rhs);
Q_ASSERT_X(lhs != rhs, Q_FUNC_INFO, "Self-assignment is not valid for move assignment.");
const auto lhsShouldDoDUChainReferenceCounting = shouldDoDUChainReferenceCounting(lhs);
const auto rhsShouldDoDUChainReferenceCounting = shouldDoDUChainReferenceCounting(rhs);
if (!lhsShouldDoDUChainReferenceCounting && !rhsShouldDoDUChainReferenceCounting) {
lhs_index = rhs_index;
rhs_index = emptyIndex;
return;
}
LockedItemRepository::write<Item>([&](auto& repo) {
if (lhs_index && lhsShouldDoDUChainReferenceCounting) {
lhs->decrease(repo.dynamicItemFromIndexSimple(lhs_index)->m_refCount, lhs_index);
} else if (rhs_index && rhsShouldDoDUChainReferenceCounting && !lhsShouldDoDUChainReferenceCounting) {
rhs->decrease(repo.dynamicItemFromIndexSimple(rhs_index)->m_refCount, rhs_index);
}
lhs_index = rhs_index;
rhs_index = emptyIndex;
if (lhs_index && lhsShouldDoDUChainReferenceCounting && !rhsShouldDoDUChainReferenceCounting) {
lhs->increase(repo.dynamicItemFromIndexSimple(lhs_index)->m_refCount, lhs_index);
}
});
}
};
}
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment