Clean up ItemRepositoryRegistry [Follow-up from "Refactor ItemRepository mutex handling"]
The following discussion from !328 (merged) should be addressed:
-
@igorkushnir started a discussion: (+3 comments) ItemRepositoryRegistry::unRegisterRepository()
- no need for a lock becauseunRegisterRepository()
is called only from within~ItemRepository()
. But there seems to be an inefficiency here:close()
is called fromunRegisterRepository()
and then immediately from~ItemRepository()
again. Should perhaps this call be removed fromunRegisterRepository()
?
More cleanup:
-
ItemRepositoryRegistry::registerRepository()
is called only fromItemRepository
's constructor, so it doesn't need to lock the mutex either. -
ItemRepositoryRegistryPrivate::open()
is called only fromItemRepositoryRegistry
's constructor. Som_repositories
is always empty in it, and they don't have to be opened there (an assertion of emptiness could be added instead). Thisopen()
can become private and be called inItemRepositoryPrivate
's constructor. -
ItemRepositoryRegistryPrivate::close()
is called only fromItemRepositoryRegistry
's destructor.ItemRepositoryRegistryPrivate::close()
can be converted into~ItemRepositoryPrivate()
and the locking ofItemRepositoryRegistryPrivate::m_mutex
can be removed from the destructors. -
ItemRepositoryRegistry
doesn't useAbstractRepositoryManager
.ItemRepository
only forwards the manager toItemRepositoryRegistry
. Therefore, remove the dependency of these two classes on (remove all mentions of)AbstractRepositoryManager
. Actually since the class is unused elsewhere too, remove the entire classAbstractRepositoryManager
. 5128ac8d made this abstract class useless like this:
~RepositoryManager() {
- deleteRepository();
+ //Don't do this, we don't need it, and it may lead to crashes
+// deleteRepository();
}
In ItemRepositoryRegistryPrivate
change the type of m_repositories
from QMap<AbstractItemRepository*, AbstractRepositoryManager*>
to std::set<AbstractItemRepository*>
and add the following comment above the declaration:
// TODO: is the order of repositories important? If not, store them in a QSet rather than std::set.