From 981ebfe48813aa759c5a1b201c9da0ab060d77da Mon Sep 17 00:00:00 2001 From: Marco Bubke Date: Thu, 21 Jul 2022 12:02:29 +0200 Subject: [PATCH] Sqlite: Cleanup id handling Because ids are now handled directly by the sqlite statement the shortcut to take the "address" of the id is deactivated and you have to use some special functions. Change-Id: I869f5d1688ab4b6794fb9ed3ffcaa3978a0fc516 Reviewed-by: Qt CI Bot Reviewed-by: Tim Jenssen --- src/libs/sqlite/sqlitebasestatement.h | 2 +- src/libs/sqlite/sqliteids.h | 14 ++- .../projectstorage/projectstorage.h | 90 +++++++++---------- .../projectstorage/projectstorageexceptions.h | 9 ++ tests/unit/unittest/gtest-creator-printing.h | 2 +- tests/unit/unittest/projectstorage-test.cpp | 5 +- .../unittest/projectstorageupdater-test.cpp | 5 +- tests/unit/unittest/sqlitestatement-test.cpp | 4 +- tests/unit/unittest/storagecache-test.cpp | 2 +- 9 files changed, 77 insertions(+), 56 deletions(-) diff --git a/src/libs/sqlite/sqlitebasestatement.h b/src/libs/sqlite/sqlitebasestatement.h index 74b0d7a6c3b..1adfd70245b 100644 --- a/src/libs/sqlite/sqlitebasestatement.h +++ b/src/libs/sqlite/sqlitebasestatement.h @@ -108,7 +108,7 @@ public: void bind(int index, Type id) { if (id) - bind(index, &id); + bind(index, id.internalId()); else bindNull(index); } diff --git a/src/libs/sqlite/sqliteids.h b/src/libs/sqlite/sqliteids.h index 3bb7378bf88..5c744f6fab5 100644 --- a/src/libs/sqlite/sqliteids.h +++ b/src/libs/sqlite/sqliteids.h @@ -50,6 +50,11 @@ public: return id; } + constexpr friend bool compareInvalidAreTrue(BasicId first, BasicId second) + { + return first.id == second.id; + } + constexpr friend bool operator==(BasicId first, BasicId second) { return first.id == second.id && first.isValid() && second.isValid(); @@ -68,13 +73,20 @@ public: return first.id >= second.id; } + constexpr friend InternalIntegerType operator-(BasicId first, BasicId second) + { + return first.id - second.id; + } + constexpr bool isValid() const { return id >= 0; } explicit operator bool() const { return isValid(); } explicit operator std::size_t() const { return static_cast(id); } - InternalIntegerType operator&() const { return id; } + InternalIntegerType internalId() const { return id; } + + [[noreturn, deprecated]] InternalIntegerType operator&() const { throw std::exception{}; } private: InternalIntegerType id = -1; diff --git a/src/plugins/qmldesigner/designercore/projectstorage/projectstorage.h b/src/plugins/qmldesigner/designercore/projectstorage/projectstorage.h index 95b57ff1544..e6490fb2e5e 100644 --- a/src/plugins/qmldesigner/designercore/projectstorage/projectstorage.h +++ b/src/plugins/qmldesigner/designercore/projectstorage/projectstorage.h @@ -675,11 +675,11 @@ private: const SourceIds &updatedProjectSourceIds) { auto compareKey = [](auto &&first, auto &&second) { - auto projectSourceIdDifference = &first.projectSourceId - &second.projectSourceId; + auto projectSourceIdDifference = first.projectSourceId - second.projectSourceId; if (projectSourceIdDifference != 0) return projectSourceIdDifference; - return &first.sourceId - &second.sourceId; + return first.sourceId - second.sourceId; }; std::sort(projectDatas.begin(), projectDatas.end(), [&](auto &&first, auto &&second) { @@ -731,13 +731,8 @@ private: void synchronizeFileStatuses(FileStatuses &fileStatuses, const SourceIds &updatedSourceIds) { - auto updatedSourceIdValues = Utils::transform(updatedSourceIds, - [](SourceId sourceId) { - return &sourceId; - }); - auto compareKey = [](auto &&first, auto &&second) { - return &first.sourceId - &second.sourceId; + return first.sourceId - second.sourceId; }; std::sort(fileStatuses.begin(), fileStatuses.end(), [&](auto &&first, auto &&second) { @@ -748,7 +743,9 @@ private: toIntegers(updatedSourceIds)); auto insert = [&](const FileStatus &fileStatus) { - insertFileStatusStatement.write(&fileStatus.sourceId, + if (!fileStatus.sourceId) + throw FileStatusHasInvalidSourceId{}; + insertFileStatusStatement.write(fileStatus.sourceId, fileStatus.size, fileStatus.lastModified); }; @@ -756,7 +753,7 @@ private: auto update = [&](const FileStatus &fileStatusFromDatabase, const FileStatus &fileStatus) { if (fileStatusFromDatabase.lastModified != fileStatus.lastModified || fileStatusFromDatabase.size != fileStatus.size) { - updateFileStatusStatement.write(&fileStatus.sourceId, + updateFileStatusStatement.write(fileStatus.sourceId, fileStatus.size, fileStatus.lastModified); return Sqlite::UpdateChange::Update; @@ -766,7 +763,7 @@ private: }; auto remove = [&](const FileStatus &fileStatus) { - deleteFileStatusStatement.write(&fileStatus.sourceId); + deleteFileStatusStatement.write(fileStatus.sourceId); }; Sqlite::insertUpdateDelete(range, fileStatuses, compareKey, insert, update, remove); @@ -805,11 +802,11 @@ private: auto compareKey = [](const Storage::Synchronization::ModuleExportedImportView &view, const Storage::Synchronization::ModuleExportedImport &import) -> long long { - auto moduleIdDifference = &view.moduleId - &import.moduleId; + auto moduleIdDifference = view.moduleId - import.moduleId; if (moduleIdDifference != 0) return moduleIdDifference; - return &view.exportedModuleId - &import.exportedModuleId; + return view.exportedModuleId - import.exportedModuleId; }; auto insert = [&](const Storage::Synchronization::ModuleExportedImport &import) { @@ -855,7 +852,7 @@ private: auto fetchModuleNameUnguarded(ModuleId id) const { - auto moduleName = selectModuleNameStatement.template value(&id); + auto moduleName = selectModuleNameStatement.template value(id); if (moduleName.empty()) throw ModuleDoesNotExists{}; @@ -898,7 +895,7 @@ private: PropertyDeclarations &relinkablePropertyDeclarations) { updatesPropertyDeclarationPropertyTypeToNullStatement.readTo(relinkablePropertyDeclarations, - &typeId); + typeId); } void handlePrototypes(TypeId prototypeId, Prototypes &relinkablePrototypes) @@ -920,7 +917,7 @@ private: handlePropertyDeclarationWithPropertyType(typeId, relinkablePropertyDeclarations); handleAliasPropertyDeclarationsWithPropertyType(typeId, relinkableAliasPropertyDeclarations); handlePrototypes(typeId, relinkablePrototypes); - deleteTypeNamesByTypeIdStatement.write(&typeId); + deleteTypeNamesByTypeIdStatement.write(typeId); deleteEnumerationDeclarationByTypeIdStatement.write(typeId); deletePropertyDeclarationByTypeIdStatement.write(typeId); deleteFunctionDeclarationByTypeIdStatement.write(typeId); @@ -1125,7 +1122,7 @@ private: auto compareKey = [](const Storage::Synchronization::ExportedTypeView &view, const Storage::Synchronization::ExportedType &type) -> long long { - auto moduleIdDifference = &view.moduleId - &type.moduleId; + auto moduleIdDifference = view.moduleId - type.moduleId; if (moduleIdDifference != 0) return moduleIdDifference; @@ -1226,8 +1223,8 @@ private: .template value(typeId, value.name); if (nextPropertyDeclarationId) { - updateAliasIdPropertyDeclarationStatement.write(&nextPropertyDeclarationId, - &propertyDeclarationId); + updateAliasIdPropertyDeclarationStatement.write(nextPropertyDeclarationId, + propertyDeclarationId); updatePropertyAliasDeclarationRecursivelyWithTypeAndTraitsStatement .write(propertyDeclarationId, propertyTypeId, value.traits); } @@ -1291,7 +1288,7 @@ private: }); auto range = selectPropertyDeclarationsForTypeIdStatement - .template range(&typeId); + .template range(typeId); auto compareKey = [](const Storage::Synchronization::PropertyDeclarationView &view, const Storage::Synchronization::PropertyDeclaration &value) { @@ -1329,7 +1326,7 @@ private: auto remove = [&](const Storage::Synchronization::PropertyDeclarationView &view) { auto nextPropertyDeclarationId = selectPropertyDeclarationIdPrototypeChainDownStatement - .template value(&typeId, + .template value(typeId, view.name); if (nextPropertyDeclarationId) { updateAliasPropertyDeclarationByAliasPropertyDeclarationIdStatement @@ -1374,7 +1371,7 @@ private: }); auto range = selectPropertyDeclarationsWithAliasForTypeIdStatement - .template range(&type.typeId); + .template range(type.typeId); auto compareKey = [](const AliasPropertyDeclarationView &view, const Storage::Synchronization::PropertyDeclaration &value) { @@ -1456,11 +1453,11 @@ private: auto compareKey = [](const Storage::Synchronization::ImportView &view, const Storage::Synchronization::Import &import) -> long long { - auto sourceIdDifference = &view.sourceId - &import.sourceId; + auto sourceIdDifference = view.sourceId - import.sourceId; if (sourceIdDifference != 0) return sourceIdDifference; - auto moduleIdDifference = &view.moduleId - &import.moduleId; + auto moduleIdDifference = view.moduleId - import.moduleId; if (moduleIdDifference != 0) return moduleIdDifference; @@ -1495,7 +1492,7 @@ private: }; selectModuleExportedImportsForModuleIdStatement.readCallback(callback, - &import.moduleId, + import.moduleId, import.version.major.value, import.version.minor.value); }; @@ -1506,7 +1503,7 @@ private: }; auto remove = [&](const Storage::Synchronization::ImportView &view) { - deleteDocumentImportStatement.write(&view.importId); + deleteDocumentImportStatement.write(view.importId); }; Sqlite::insertUpdateDelete(range, imports, compareKey, insert, update, remove); @@ -1559,7 +1556,7 @@ private: }); auto range = selectFunctionDeclarationsForTypeIdStatement - .template range(&typeId); + .template range(typeId); auto compareKey = [](const Storage::Synchronization::FunctionDeclarationView &view, const Storage::Synchronization::FunctionDeclaration &value) { @@ -1575,7 +1572,7 @@ private: auto insert = [&](const Storage::Synchronization::FunctionDeclaration &value) { Utils::PathString signature{createJson(value.parameters)}; - insertFunctionDeclarationStatement.write(&typeId, value.name, value.returnTypeName, signature); + insertFunctionDeclarationStatement.write(typeId, value.name, value.returnTypeName, signature); }; auto update = [&](const Storage::Synchronization::FunctionDeclarationView &view, @@ -1585,13 +1582,13 @@ private: if (value.returnTypeName == view.returnTypeName) return Sqlite::UpdateChange::No; - updateFunctionDeclarationStatement.write(&view.id, value.returnTypeName); + updateFunctionDeclarationStatement.write(view.id, value.returnTypeName); return Sqlite::UpdateChange::Update; }; auto remove = [&](const Storage::Synchronization::FunctionDeclarationView &view) { - deleteFunctionDeclarationStatement.write(&view.id); + deleteFunctionDeclarationStatement.write(view.id); }; Sqlite::insertUpdateDelete(range, functionsDeclarations, compareKey, insert, update, remove); @@ -1614,7 +1611,7 @@ private: }); auto range = selectSignalDeclarationsForTypeIdStatement - .template range(&typeId); + .template range(typeId); auto compareKey = [](const Storage::Synchronization::SignalDeclarationView &view, const Storage::Synchronization::SignalDeclaration &value) { @@ -1630,7 +1627,7 @@ private: auto insert = [&](const Storage::Synchronization::SignalDeclaration &value) { Utils::PathString signature{createJson(value.parameters)}; - insertSignalDeclarationStatement.write(&typeId, value.name, signature); + insertSignalDeclarationStatement.write(typeId, value.name, signature); }; auto update = [&]([[maybe_unused]] const Storage::Synchronization::SignalDeclarationView &view, @@ -1639,7 +1636,7 @@ private: }; auto remove = [&](const Storage::Synchronization::SignalDeclarationView &view) { - deleteSignalDeclarationStatement.write(&view.id); + deleteSignalDeclarationStatement.write(view.id); }; Sqlite::insertUpdateDelete(range, signalDeclarations, compareKey, insert, update, remove); @@ -1681,7 +1678,7 @@ private: }); auto range = selectEnumerationDeclarationsForTypeIdStatement - .template range(&typeId); + .template range(typeId); auto compareKey = [](const Storage::Synchronization::EnumerationDeclarationView &view, const Storage::Synchronization::EnumerationDeclaration &value) { @@ -1727,17 +1724,17 @@ private: TypeId declareType(Storage::Synchronization::Type &type) { if (type.typeName.isEmpty()) { - type.typeId = selectTypeIdBySourceIdStatement.template value(&type.sourceId); + type.typeId = selectTypeIdBySourceIdStatement.template value(type.sourceId); return type.typeId; } - type.typeId = upsertTypeStatement.template value(&type.sourceId, + type.typeId = upsertTypeStatement.template value(type.sourceId, type.typeName, type.accessSemantics); if (!type.typeId) - type.typeId = selectTypeIdBySourceIdAndNameStatement.template value(&type.sourceId, + type.typeId = selectTypeIdBySourceIdAndNameStatement.template value(type.sourceId, type.typeName); return type.typeId; @@ -1819,7 +1816,7 @@ private: auto compareKey = [](const TypeWithDefaultPropertyView &view, const Storage::Synchronization::Type &value) { - return &view.typeId - &value.typeId; + return view.typeId - value.typeId; }; auto insert = [&](const Storage::Synchronization::Type &) { @@ -1834,7 +1831,7 @@ private: value.typeId, value.defaultPropertyName) .propertyDeclarationId; - if (&valueDefaultPropertyId == &view.defaultPropertyId) + if (compareInvalidAreTrue(valueDefaultPropertyId, view.defaultPropertyId)) return Sqlite::UpdateChange::No; updateDefaultPropertyIdStatement.write(value.typeId, valueDefaultPropertyId); @@ -1853,7 +1850,7 @@ private: auto compareKey = [](const TypeWithDefaultPropertyView &view, const Storage::Synchronization::Type &value) { - return &view.typeId - &value.typeId; + return view.typeId - value.typeId; }; auto insert = [&](const Storage::Synchronization::Type &) { @@ -1870,7 +1867,7 @@ private: valueDefaultPropertyId = optionalValueDefaultPropertyId->propertyDeclarationId; } - if (&valueDefaultPropertyId == &view.defaultPropertyId) + if (compareInvalidAreTrue(valueDefaultPropertyId, view.defaultPropertyId)) return Sqlite::UpdateChange::No; updateDefaultPropertyIdStatement.write(value.typeId, Sqlite::NullValue{}); @@ -1915,7 +1912,7 @@ private: if (Utils::visit([](auto &&typeName) -> bool { return typeName.name.isEmpty(); }, type.prototype)) { - updatePrototypeStatement.write(&type.typeId, Sqlite::NullValue{}, Sqlite::NullValue{}); + updatePrototypeStatement.write(type.typeId, Sqlite::NullValue{}, Sqlite::NullValue{}); } else { ImportedTypeNameId prototypeTypeNameId = fetchImportedTypeNameId(type.prototype, type.sourceId); @@ -1967,7 +1964,7 @@ private: auto operator()(const Storage::Synchronization::ImportedType &importedType) { return storage.fetchImportedTypeNameId(Storage::Synchronization::TypeNameKind::Exported, - &sourceId, + sourceId, importedType.name); } @@ -1976,7 +1973,7 @@ private: ImportId importId = storage.fetchImportId(sourceId, importedType.import); return storage.fetchImportedTypeNameId(Storage::Synchronization::TypeNameKind::QualifiedExported, - &importId, + importId, importedType.name); } @@ -1987,8 +1984,9 @@ private: return Utils::visit(Inspect{*this, sourceId}, name); } + template ImportedTypeNameId fetchImportedTypeNameId(Storage::Synchronization::TypeNameKind kind, - long long id, + Id id, Utils::SmallStringView typeName) { auto importedTypeNameId = selectImportedTypeNameIdStatement @@ -2155,7 +2153,7 @@ private: }; selectEnumerationDeclarationsForTypeIdWithoutEnumeratorDeclarationsStatement - .readCallback(callback, &typeId); + .readCallback(callback, typeId); return enumerationDeclarations; } diff --git a/src/plugins/qmldesigner/designercore/projectstorage/projectstorageexceptions.h b/src/plugins/qmldesigner/designercore/projectstorage/projectstorageexceptions.h index 1c37a03978f..5c1c58f9134 100644 --- a/src/plugins/qmldesigner/designercore/projectstorage/projectstorageexceptions.h +++ b/src/plugins/qmldesigner/designercore/projectstorage/projectstorageexceptions.h @@ -143,4 +143,13 @@ public: const char *what() const noexcept override { return "The module id is invalid!"; } }; +class FileStatusHasInvalidSourceId : std::exception +{ +public: + const char *what() const noexcept override + { + return "The source id in file status is invalid!"; + } +}; + } // namespace QmlDesigner diff --git a/tests/unit/unittest/gtest-creator-printing.h b/tests/unit/unittest/gtest-creator-printing.h index 3f774011d02..1dc6431cac6 100644 --- a/tests/unit/unittest/gtest-creator-printing.h +++ b/tests/unit/unittest/gtest-creator-printing.h @@ -56,7 +56,7 @@ std::ostream &operator<<(std::ostream &out, TimeStamp timeStamp); template std::ostream &operator<<(std::ostream &out, const BasicId &id) { - return out << "id=" << &id; + return out << "id=" << id.internalId(); } namespace SessionChangeSetInternal { diff --git a/tests/unit/unittest/projectstorage-test.cpp b/tests/unit/unittest/projectstorage-test.cpp index 6a5fc7c4159..2da930a3f6a 100644 --- a/tests/unit/unittest/projectstorage-test.cpp +++ b/tests/unit/unittest/projectstorage-test.cpp @@ -97,7 +97,8 @@ MATCHER_P4(IsStorageType, const Storage::Synchronization::Type &type = arg; return type.sourceId == sourceId && type.typeName == typeName - && type.accessSemantics == accessSemantics && &prototypeId == &type.prototypeId; + && type.accessSemantics == accessSemantics + && compareInvalidAreTrue(prototypeId, type.prototypeId); } MATCHER_P(IsExportedType, @@ -3864,7 +3865,7 @@ TEST_F(ProjectStorage, ThrowForInvalidSourceIdInFileStatus) FileStatus fileStatus1{SourceId{}, 100, 100}; ASSERT_THROW(storage.synchronize(SynchronizationPackage{{sourceId1}, {fileStatus1}}), - Sqlite::ConstraintPreventsModification); + QmlDesigner::FileStatusHasInvalidSourceId); } TEST_F(ProjectStorage, FetchAllFileStatuses) diff --git a/tests/unit/unittest/projectstorageupdater-test.cpp b/tests/unit/unittest/projectstorageupdater-test.cpp index 180c14658f2..70f45034239 100644 --- a/tests/unit/unittest/projectstorageupdater-test.cpp +++ b/tests/unit/unittest/projectstorageupdater-test.cpp @@ -125,8 +125,9 @@ MATCHER_P4(IsProjectData, { const Storage::Synchronization::ProjectData &projectData = arg; - return &projectData.projectSourceId == &projectSourceId && projectData.sourceId == sourceId - && projectData.moduleId == moduleId && projectData.fileType == fileType; + return compareInvalidAreTrue(projectData.projectSourceId, projectSourceId) + && projectData.sourceId == sourceId && projectData.moduleId == moduleId + && projectData.fileType == fileType; } MATCHER(PackageIsEmpty, std::string(negation ? "isn't empty" : "is empty")) diff --git a/tests/unit/unittest/sqlitestatement-test.cpp b/tests/unit/unittest/sqlitestatement-test.cpp index a30c397097a..8c49b563619 100644 --- a/tests/unit/unittest/sqlitestatement-test.cpp +++ b/tests/unit/unittest/sqlitestatement-test.cpp @@ -1248,7 +1248,7 @@ TEST_F(SqliteStatement, GetSingleLongLongId) auto value = statement.value(); - ASSERT_THAT(&value, Eq(42)); + ASSERT_THAT(value.internalId(), Eq(42)); } TEST_F(SqliteStatement, GetSingleInvalidIntId) @@ -1270,7 +1270,7 @@ TEST_F(SqliteStatement, GetSingleIntId) auto value = statement.value(); - ASSERT_THAT(&value, Eq(42)); + ASSERT_THAT(value.internalId(), Eq(42)); } TEST_F(SqliteStatement, GetValueCallsReset) diff --git a/tests/unit/unittest/storagecache-test.cpp b/tests/unit/unittest/storagecache-test.cpp index 712f907ba3d..31c62c7b54f 100644 --- a/tests/unit/unittest/storagecache-test.cpp +++ b/tests/unit/unittest/storagecache-test.cpp @@ -442,7 +442,7 @@ TYPED_TEST(StorageCache, GetEntryByIndexAfterInsertingByCustomIndex) TYPED_TEST(StorageCache, CallFetchSourceContextPathForLowerIndex) { auto index = this->cache.id("foo"); - SourceContextId lowerIndex{SourceContextId::create(&index - 1)}; + SourceContextId lowerIndex{SourceContextId::create(index.internalId() - 1)}; EXPECT_CALL(this->mockStorage, fetchSourceContextPath(Eq(lowerIndex)));