From b5fdfc2474942925006777e30e3940eeaa870df3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sat, 1 Aug 2020 13:20:13 +0200 Subject: [PATCH] Trade: check for custom MaterialData deleters in AbstractImporter. And adapt MaterialData and the deprecated PhongMaterialData constructor to follow these rules. Basically the same as done for ImageData and MeshData. --- src/Magnum/Trade/AbstractImporter.cpp | 4 + src/Magnum/Trade/MaterialData.cpp | 3 +- src/Magnum/Trade/MaterialData.h | 5 ++ src/Magnum/Trade/PhongMaterialData.cpp | 3 + .../Trade/Test/AbstractImporterTest.cpp | 89 +++++++++++++++++++ 5 files changed, 103 insertions(+), 1 deletion(-) diff --git a/src/Magnum/Trade/AbstractImporter.cpp b/src/Magnum/Trade/AbstractImporter.cpp index 404c275b9..0fda5dbd8 100644 --- a/src/Magnum/Trade/AbstractImporter.cpp +++ b/src/Magnum/Trade/AbstractImporter.cpp @@ -681,6 +681,10 @@ AbstractImporter::material(const UnsignedInt id) { CORRADE_ASSERT(id < doMaterialCount(), "Trade::AbstractImporter::material(): index" << id << "out of range for" << doMaterialCount() << "entries", {}); Containers::Optional material = doMaterial(id); + CORRADE_ASSERT(!material || ( + (!material->_data.deleter() || material->_data.deleter() == reinterpret_cast(Implementation::nonOwnedArrayDeleter)) && + (!material->_layerOffsets.deleter() || material->_layerOffsets.deleter() == reinterpret_cast(Implementation::nonOwnedArrayDeleter))), + "Trade::AbstractImporter::material(): implementation is not allowed to use a custom Array deleter", {}); /* GCC 4.8 and clang-cl needs an explicit conversion here */ #ifdef MAGNUM_BUILD_DEPRECATED diff --git a/src/Magnum/Trade/MaterialData.cpp b/src/Magnum/Trade/MaterialData.cpp index 2106b1acd..a5ad23ba4 100644 --- a/src/Magnum/Trade/MaterialData.cpp +++ b/src/Magnum/Trade/MaterialData.cpp @@ -32,6 +32,7 @@ #include "Magnum/Math/Vector4.h" #include "Magnum/Math/Matrix.h" +#include "Magnum/Trade/Data.h" #include "Magnum/Trade/Implementation/arrayUtilities.h" namespace Magnum { namespace Trade { @@ -226,7 +227,7 @@ MaterialData::MaterialData(const MaterialTypes types, Containers::Array attributeData, const std::initializer_list layerData, const void* const importerState): MaterialData{types, Implementation::initializerListToArrayWithDefaultDeleter(attributeData), Implementation::initializerListToArrayWithDefaultDeleter(layerData), importerState} {} -MaterialData::MaterialData(const MaterialTypes types, DataFlags, const Containers::ArrayView attributeData, DataFlags, Containers::ArrayView layerData, const void* const importerState) noexcept: _data{Containers::Array{const_cast(attributeData.data()), attributeData.size(), [](MaterialAttributeData*, std::size_t){}}}, _layerOffsets{Containers::Array{const_cast(layerData.data()), layerData.size(), [](UnsignedInt*, std::size_t){}}}, _types{types}, _importerState{importerState} { +MaterialData::MaterialData(const MaterialTypes types, DataFlags, const Containers::ArrayView attributeData, DataFlags, Containers::ArrayView layerData, const void* const importerState) noexcept: _data{Containers::Array{const_cast(attributeData.data()), attributeData.size(), reinterpret_cast(Implementation::nonOwnedArrayDeleter)}}, _layerOffsets{Containers::Array{const_cast(layerData.data()), layerData.size(), reinterpret_cast(Implementation::nonOwnedArrayDeleter)}}, _types{types}, _importerState{importerState} { #ifndef CORRADE_NO_ASSERT /* Not checking what's already done in MaterialAttributeData constructor */ for(std::size_t i = 0; i != _data.size(); ++i) diff --git a/src/Magnum/Trade/MaterialData.h b/src/Magnum/Trade/MaterialData.h index 1fe44c733..7b9d8e8cf 100644 --- a/src/Magnum/Trade/MaterialData.h +++ b/src/Magnum/Trade/MaterialData.h @@ -1498,6 +1498,11 @@ class MAGNUM_TRADE_EXPORT MaterialData { const void* importerState() const { return _importerState; } private: + /* For custom deleter checks. Not done in the constructors here because + the restriction is pointless when used outside of plugin + implementations. */ + friend AbstractImporter; + static Containers::StringView attributeString(MaterialAttribute name); /* Internal helpers that don't assert, unlike layerId() / attributeId() */ UnsignedInt layerFor(Containers::StringView layer) const; diff --git a/src/Magnum/Trade/PhongMaterialData.cpp b/src/Magnum/Trade/PhongMaterialData.cpp index bf0b48915..2c596b4a3 100644 --- a/src/Magnum/Trade/PhongMaterialData.cpp +++ b/src/Magnum/Trade/PhongMaterialData.cpp @@ -91,6 +91,9 @@ PhongMaterialData::PhongMaterialData(const Flags flags, const Color4& ambientCol arrayAppend(data, Containers::InPlaceInit, MaterialAttribute::Shininess, shininess); + /* Convert back to a non-growable Array as importers don't allow custom + deleters in plugin implementations */ + arrayShrink(data, Containers::DefaultInit); return data; }(), importerState} { /* The data can't be filled here because it won't be sorted correctly */ diff --git a/src/Magnum/Trade/Test/AbstractImporterTest.cpp b/src/Magnum/Trade/Test/AbstractImporterTest.cpp index d9f0f1009..4a69917d1 100644 --- a/src/Magnum/Trade/Test/AbstractImporterTest.cpp +++ b/src/Magnum/Trade/Test/AbstractImporterTest.cpp @@ -197,6 +197,9 @@ struct AbstractImporterTest: TestSuite::Tester { void materialNameOutOfRange(); void materialNotImplemented(); void materialOutOfRange(); + void materialNonOwningDeleters(); + void materialCustomAttributeDataDeleter(); + void materialCustomLayerDataDeleter(); void texture(); void textureNameNotImplemented(); @@ -402,6 +405,9 @@ AbstractImporterTest::AbstractImporterTest() { &AbstractImporterTest::materialNameOutOfRange, &AbstractImporterTest::materialNotImplemented, &AbstractImporterTest::materialOutOfRange, + &AbstractImporterTest::materialNonOwningDeleters, + &AbstractImporterTest::materialCustomAttributeDataDeleter, + &AbstractImporterTest::materialCustomLayerDataDeleter, &AbstractImporterTest::texture, &AbstractImporterTest::textureNameNotImplemented, @@ -3131,6 +3137,89 @@ void AbstractImporterTest::materialOutOfRange() { CORRADE_COMPARE(out.str(), "Trade::AbstractImporter::material(): index 8 out of range for 8 entries\n"); } +void AbstractImporterTest::materialNonOwningDeleters() { + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doMaterialCount() const override { return 1; } + Containers::Optional doMaterial(UnsignedInt) override { + return MaterialData{{}, {}, attributeData, {}, layerData}; + } + + UnsignedInt layerData[1]{}; + MaterialAttributeData attributeData[1]{ + {MaterialAttribute::DiffuseColor, Color4{}} + }; + } importer; + + auto data = importer.material(0); + CORRADE_VERIFY(data); + CORRADE_COMPARE(static_cast(data->attributeData()), importer.attributeData); + CORRADE_COMPARE(static_cast(data->layerData()), importer.layerData); +} + +void AbstractImporterTest::materialCustomAttributeDataDeleter() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doMaterialCount() const override { return 1; } + Int doMaterialForName(const std::string&) override { return 0; } + Containers::Optional doMaterial(UnsignedInt) override { + return MaterialData{{}, Containers::Array{attributeData, 1, [](MaterialAttributeData*, std::size_t) {}}}; + } + + MaterialAttributeData attributeData[1]{ + {MaterialAttribute::DiffuseColor, Color4{}} + }; + } importer; + + std::ostringstream out; + Error redirectError{&out}; + + importer.material(0); + importer.material(""); + CORRADE_COMPARE(out.str(), + "Trade::AbstractImporter::material(): implementation is not allowed to use a custom Array deleter\n" + "Trade::AbstractImporter::material(): implementation is not allowed to use a custom Array deleter\n"); +} + +void AbstractImporterTest::materialCustomLayerDataDeleter() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + struct: AbstractImporter { + ImporterFeatures doFeatures() const override { return {}; } + bool doIsOpened() const override { return true; } + void doClose() override {} + + UnsignedInt doMaterialCount() const override { return 1; } + Int doMaterialForName(const std::string&) override { return 0; } + Containers::Optional doMaterial(UnsignedInt) override { + return MaterialData{{}, nullptr, Containers::Array{layerData, 1, [](UnsignedInt*, std::size_t) {}}}; + } + + UnsignedInt layerData[1]{}; + } importer; + + std::ostringstream out; + Error redirectError{&out}; + + importer.material(0); + importer.material(""); + CORRADE_COMPARE(out.str(), + "Trade::AbstractImporter::material(): implementation is not allowed to use a custom Array deleter\n" + "Trade::AbstractImporter::material(): implementation is not allowed to use a custom Array deleter\n"); +} + void AbstractImporterTest::texture() { struct: AbstractImporter { ImporterFeatures doFeatures() const override { return {}; }