Skip to content

Commit

Permalink
Trade: allow non-owning aray deleters passed through Importer APIs.
Browse files Browse the repository at this point in the history
  • Loading branch information
@mosra
mosra committed Nov 13, 2019
1 parent 8c43eaf commit 0281b82
Show file tree
Hide file tree
Showing 3 changed files with 130 additions and 8 deletions.
17 changes: 12 additions & 5 deletions src/Magnum/Trade/AbstractImporter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,10 @@ Containers::Optional<AnimationData> AbstractImporter::animation(const UnsignedIn
CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::animation(): no file opened", {});
CORRADE_ASSERT(id < doAnimationCount(), "Trade::AbstractImporter::animation(): index" << id << "out of range for" << doAnimationCount() << "entries", {});
Containers::Optional<AnimationData> animation = doAnimation(id);
CORRADE_ASSERT(!animation || (!animation->_data.deleter() && !animation->_tracks.deleter()), "Trade::AbstractImporter::animation(): implementation is not allowed to use a custom Array deleter", {});
CORRADE_ASSERT(!animation ||
((!animation->_data.deleter() || animation->_data.deleter() == Implementation::nonOwnedArrayDeleter) &&
(!animation->_tracks.deleter() || animation->_tracks.deleter() == reinterpret_cast<void(*)(AnimationTrackData*, std::size_t)>(Implementation::nonOwnedArrayDeleter))),
"Trade::AbstractImporter::animation(): implementation is not allowed to use a custom Array deleter", {});
return animation;
}

Expand Down Expand Up @@ -429,7 +432,11 @@ Containers::Optional<MeshData> AbstractImporter::mesh(const UnsignedInt id) {
CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::mesh(): no file opened", {});
CORRADE_ASSERT(id < doMeshCount(), "Trade::AbstractImporter::mesh(): index" << id << "out of range for" << doMeshCount() << "entries", {});
Containers::Optional<MeshData> mesh = doMesh(id);
CORRADE_ASSERT(!mesh || (!mesh->_indexData.deleter() && !mesh->_vertexData.deleter() && !mesh->_attributes.deleter()), "Trade::AbstractImporter::mesh(): implementation is not allowed to use a custom Array deleter", {});
CORRADE_ASSERT(!mesh || (
(!mesh->_indexData.deleter() || mesh->_indexData.deleter() == Implementation::nonOwnedArrayDeleter) &&
(!mesh->_vertexData.deleter() || mesh->_vertexData.deleter() == Implementation::nonOwnedArrayDeleter) &&
(!mesh->_attributes.deleter()|| mesh->_attributes.deleter() == reinterpret_cast<void(*)(MeshAttributeData*, std::size_t)>(Implementation::nonOwnedArrayDeleter))),
"Trade::AbstractImporter::mesh(): implementation is not allowed to use a custom Array deleter", {});
return mesh;
}

Expand Down Expand Up @@ -591,7 +598,7 @@ Containers::Optional<ImageData1D> AbstractImporter::image1D(const UnsignedInt id
CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::image1D(): no file opened", {});
CORRADE_ASSERT(id < doImage1DCount(), "Trade::AbstractImporter::image1D(): index" << id << "out of range for" << doImage1DCount() << "entries", {});
Containers::Optional<ImageData1D> image = doImage1D(id);
CORRADE_ASSERT(!image || !image->_data.deleter(), "Trade::AbstractImporter::image1D(): implementation is not allowed to use a custom Array deleter", {});
CORRADE_ASSERT(!image || !image->_data.deleter() || image->_data.deleter() == Implementation::nonOwnedArrayDeleter, "Trade::AbstractImporter::image1D(): implementation is not allowed to use a custom Array deleter", {});
return image;
}

Expand Down Expand Up @@ -625,7 +632,7 @@ Containers::Optional<ImageData2D> AbstractImporter::image2D(const UnsignedInt id
CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::image2D(): no file opened", {});
CORRADE_ASSERT(id < doImage2DCount(), "Trade::AbstractImporter::image2D(): index" << id << "out of range for" << doImage2DCount() << "entries", {});
Containers::Optional<ImageData2D> image = doImage2D(id);
CORRADE_ASSERT(!image || !image->_data.deleter(), "Trade::AbstractImporter::image2D(): implementation is not allowed to use a custom Array deleter", {});
CORRADE_ASSERT(!image || !image->_data.deleter() || image->_data.deleter() == Implementation::nonOwnedArrayDeleter, "Trade::AbstractImporter::image2D(): implementation is not allowed to use a custom Array deleter", {});
return image;
}

Expand Down Expand Up @@ -659,7 +666,7 @@ Containers::Optional<ImageData3D> AbstractImporter::image3D(const UnsignedInt id
CORRADE_ASSERT(isOpened(), "Trade::AbstractImporter::image3D(): no file opened", {});
CORRADE_ASSERT(id < doImage3DCount(), "Trade::AbstractImporter::image3D(): index" << id << "out of range for" << doImage3DCount() << "entries", {});
Containers::Optional<ImageData3D> image = doImage3D(id);
CORRADE_ASSERT(!image || !image->_data.deleter(), "Trade::AbstractImporter::image3D(): implementation is not allowed to use a custom Array deleter", {});
CORRADE_ASSERT(!image || !image->_data.deleter() || image->_data.deleter() == Implementation::nonOwnedArrayDeleter, "Trade::AbstractImporter::image3D(): implementation is not allowed to use a custom Array deleter", {});
return image;
}

Expand Down
8 changes: 5 additions & 3 deletions src/Magnum/Trade/AbstractImporter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,11 @@ dependency on the importer instance and neither on the dynamic plugin module.
In other words, you don't need to keep the importer instance (or the plugin
manager instance) around in order to have the `*Data` instances valid.
Moreover, all @ref Corrade::Containers::Array instances returned through
@ref ImageData, @ref AnimationData and others are only allowed to have default
deleters --- this is to avoid potential dangling function pointer calls when
destructing such instances after the plugin module has been unloaded.
@ref ImageData, @ref AnimationData and @ref MeshData are only allowed to have
default deleters (or be non-owning instances created from
@ref Corrade::Containers::ArrayView) --- this is to avoid potential dangling
function pointer calls when destructing such instances after the plugin module
has been unloaded.
The only exception are various `importerState()` functions
@ref Trade-AbstractImporter-usage-state "described above", but in that case the
Expand Down
113 changes: 113 additions & 0 deletions src/Magnum/Trade/Test/AbstractImporterTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ struct AbstractImporterTest: TestSuite::Tester {
void animationNotImplemented();
void animationNoFile();
void animationOutOfRange();
void animationNonOwningDeleters();
void animationCustomDataDeleter();
void animationCustomTrackDeleter();

Expand Down Expand Up @@ -168,6 +169,7 @@ struct AbstractImporterTest: TestSuite::Tester {
void meshNotImplemented();
void meshNoFile();
void meshOutOfRange();
void meshNonOwningDeleters();
void meshCustomIndexDataDeleter();
void meshCustomVertexDataDeleter();
void meshCustomAttributesDeleter();
Expand Down Expand Up @@ -231,6 +233,7 @@ struct AbstractImporterTest: TestSuite::Tester {
void image1DNotImplemented();
void image1DNoFile();
void image1DOutOfRange();
void image1DNonOwningDeleter();
void image1DCustomDeleter();

void image2D();
Expand All @@ -244,6 +247,7 @@ struct AbstractImporterTest: TestSuite::Tester {
void image2DNotImplemented();
void image2DNoFile();
void image2DOutOfRange();
void image2DNonOwningDeleter();
void image2DCustomDeleter();

void image3D();
Expand All @@ -257,6 +261,7 @@ struct AbstractImporterTest: TestSuite::Tester {
void image3DNotImplemented();
void image3DNoFile();
void image3DOutOfRange();
void image3DNonOwningDeleter();
void image3DCustomDeleter();

void importerState();
Expand Down Expand Up @@ -321,6 +326,7 @@ AbstractImporterTest::AbstractImporterTest() {
&AbstractImporterTest::animationNotImplemented,
&AbstractImporterTest::animationNoFile,
&AbstractImporterTest::animationOutOfRange,
&AbstractImporterTest::animationNonOwningDeleters,
&AbstractImporterTest::animationCustomDataDeleter,
&AbstractImporterTest::animationCustomTrackDeleter,

Expand Down Expand Up @@ -383,6 +389,7 @@ AbstractImporterTest::AbstractImporterTest() {
&AbstractImporterTest::meshNotImplemented,
&AbstractImporterTest::meshNoFile,
&AbstractImporterTest::meshOutOfRange,
&AbstractImporterTest::meshNonOwningDeleters,
&AbstractImporterTest::meshCustomIndexDataDeleter,
&AbstractImporterTest::meshCustomVertexDataDeleter,
&AbstractImporterTest::meshCustomAttributesDeleter,
Expand Down Expand Up @@ -446,6 +453,7 @@ AbstractImporterTest::AbstractImporterTest() {
&AbstractImporterTest::image1DNotImplemented,
&AbstractImporterTest::image1DNoFile,
&AbstractImporterTest::image1DOutOfRange,
&AbstractImporterTest::image1DNonOwningDeleter,
&AbstractImporterTest::image1DCustomDeleter,

&AbstractImporterTest::image2D,
Expand All @@ -459,6 +467,7 @@ AbstractImporterTest::AbstractImporterTest() {
&AbstractImporterTest::image2DNotImplemented,
&AbstractImporterTest::image2DNoFile,
&AbstractImporterTest::image2DOutOfRange,
&AbstractImporterTest::image2DNonOwningDeleter,
&AbstractImporterTest::image2DCustomDeleter,

&AbstractImporterTest::image3D,
Expand All @@ -472,6 +481,7 @@ AbstractImporterTest::AbstractImporterTest() {
&AbstractImporterTest::image3DNotImplemented,
&AbstractImporterTest::image3DNoFile,
&AbstractImporterTest::image3DOutOfRange,
&AbstractImporterTest::image3DNonOwningDeleter,
&AbstractImporterTest::image3DCustomDeleter,

&AbstractImporterTest::importerState,
Expand Down Expand Up @@ -1330,6 +1340,28 @@ void AbstractImporterTest::animationOutOfRange() {
CORRADE_COMPARE(out.str(), "Trade::AbstractImporter::animation(): index 8 out of range for 8 entries\n");
}

void AbstractImporterTest::animationNonOwningDeleters() {
struct: AbstractImporter {
Features doFeatures() const override { return {}; }
bool doIsOpened() const override { return true; }
void doClose() override {}

UnsignedInt doAnimationCount() const override { return 1; }
Containers::Optional<AnimationData> doAnimation(UnsignedInt) override {
return AnimationData{Containers::Array<char>{data, 1, Implementation::nonOwnedArrayDeleter},
Containers::Array<AnimationTrackData>{&track, 1,
reinterpret_cast<void(*)(AnimationTrackData*, std::size_t)>(Implementation::nonOwnedArrayDeleter)}};
}

char data[1];
AnimationTrackData track;
} importer;

auto data = importer.animation(0);
CORRADE_VERIFY(data);
CORRADE_COMPARE(static_cast<const void*>(data->data()), importer.data);
}

void AbstractImporterTest::animationCustomDataDeleter() {
struct: AbstractImporter {
Features doFeatures() const override { return {}; }
Expand Down Expand Up @@ -2198,6 +2230,30 @@ void AbstractImporterTest::meshOutOfRange() {
CORRADE_COMPARE(out.str(), "Trade::AbstractImporter::mesh(): index 8 out of range for 8 entries\n");
}

void AbstractImporterTest::meshNonOwningDeleters() {
struct: AbstractImporter {
Features doFeatures() const override { return {}; }
bool doIsOpened() const override { return true; }
void doClose() override {}

UnsignedInt doMeshCount() const override { return 1; }
Containers::Optional<MeshData> doMesh(UnsignedInt) override {
return MeshData{MeshPrimitive::Triangles,
Containers::Array<char>{indexData, 1, Implementation::nonOwnedArrayDeleter}, MeshIndexData{MeshIndexType::UnsignedByte, indexData},
Containers::Array<char>{nullptr, 0, Implementation::nonOwnedArrayDeleter},
Containers::Array<MeshAttributeData>{&positions, 1,
reinterpret_cast<void(*)(MeshAttributeData*, std::size_t)>(Implementation::nonOwnedArrayDeleter)}};
}

char indexData[1];
MeshAttributeData positions{MeshAttributeName::Positions, MeshAttributeType::Vector3, nullptr};
} importer;

auto data = importer.mesh(0);
CORRADE_VERIFY(data);
CORRADE_COMPARE(static_cast<const void*>(data->indexData()), importer.indexData);
}

void AbstractImporterTest::meshCustomIndexDataDeleter() {
struct: AbstractImporter {
Features doFeatures() const override { return {}; }
Expand Down Expand Up @@ -3108,6 +3164,25 @@ void AbstractImporterTest::image1DCustomDeleter() {
CORRADE_COMPARE(out.str(), "Trade::AbstractImporter::image1D(): implementation is not allowed to use a custom Array deleter\n");
}

void AbstractImporterTest::image1DNonOwningDeleter() {
struct: AbstractImporter {
Features doFeatures() const override { return {}; }
bool doIsOpened() const override { return true; }
void doClose() override {}

UnsignedInt doImage1DCount() const override { return 1; }
Containers::Optional<ImageData1D> doImage1D(UnsignedInt) override {
return ImageData1D{PixelFormat::RGBA8Unorm, {}, Containers::Array<char>{data, 1, Implementation::nonOwnedArrayDeleter}};
}

char data[1];
} importer;

auto data = importer.image1D(0);
CORRADE_VERIFY(data);
CORRADE_COMPARE(static_cast<const void*>(data->data()), importer.data);
}

void AbstractImporterTest::image2D() {
struct: AbstractImporter {
Features doFeatures() const override { return {}; }
Expand Down Expand Up @@ -3293,6 +3368,25 @@ void AbstractImporterTest::image2DCustomDeleter() {
CORRADE_COMPARE(out.str(), "Trade::AbstractImporter::image2D(): implementation is not allowed to use a custom Array deleter\n");
}

void AbstractImporterTest::image2DNonOwningDeleter() {
struct: AbstractImporter {
Features doFeatures() const override { return {}; }
bool doIsOpened() const override { return true; }
void doClose() override {}

UnsignedInt doImage2DCount() const override { return 1; }
Containers::Optional<ImageData2D> doImage2D(UnsignedInt) override {
return ImageData2D{PixelFormat::RGBA8Unorm, {}, Containers::Array<char>{data, 1, Implementation::nonOwnedArrayDeleter}};
}

char data[1];
} importer;

auto data = importer.image2D(0);
CORRADE_VERIFY(data);
CORRADE_COMPARE(static_cast<const void*>(data->data()), importer.data);
}

void AbstractImporterTest::image3D() {
struct: AbstractImporter {
Features doFeatures() const override { return {}; }
Expand Down Expand Up @@ -3459,6 +3553,25 @@ void AbstractImporterTest::image3DOutOfRange() {
CORRADE_COMPARE(out.str(), "Trade::AbstractImporter::image3D(): index 8 out of range for 8 entries\n");
}

void AbstractImporterTest::image3DNonOwningDeleter() {
struct: AbstractImporter {
Features doFeatures() const override { return {}; }
bool doIsOpened() const override { return true; }
void doClose() override {}

UnsignedInt doImage3DCount() const override { return 1; }
Containers::Optional<ImageData3D> doImage3D(UnsignedInt) override {
return ImageData3D{PixelFormat::RGBA8Unorm, {}, Containers::Array<char>{data, 1, Implementation::nonOwnedArrayDeleter}};
}

char data[1];
} importer;

auto data = importer.image3D(0);
CORRADE_VERIFY(data);
CORRADE_COMPARE(static_cast<const void*>(data->data()), importer.data);
}

void AbstractImporterTest::image3DCustomDeleter() {
struct: AbstractImporter {
Features doFeatures() const override { return {}; }
Expand Down

0 comments on commit 0281b82

Please sign in to comment.