From 069c81b9cbc4e99fde39b422ecb4904a17a9ccbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 5 Aug 2019 21:48:17 +0200 Subject: [PATCH] Deprecate passing nullptr to images. Causes too much pain. Ugh. --- doc/changelog.dox | 3 + .../DebugTools/Test/CompareImageTest.cpp | 10 +- src/Magnum/ImageView.cpp | 8 ++ src/Magnum/Test/ImageViewTest.cpp | 109 +++--------------- .../Text/Test/AbstractGlyphCacheTest.cpp | 8 +- .../Trade/Test/AbstractImageConverterTest.cpp | 8 +- 6 files changed, 38 insertions(+), 108 deletions(-) diff --git a/doc/changelog.dox b/doc/changelog.dox index 0816e0814..25acac4ec 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -550,6 +550,9 @@ See also: use the non-templated version together with @ref Corrade::Containers::arrayCast() instead for properly bounds-checked type conversion +- Passing @cpp nullptr @ce to @ref ImageView constructors is deprecated and + will print a warning at runtime. Use a constructor without the @p data + parameter instead. @subsection changelog-latest-compatibility Potential compatibility breakages, removed APIs diff --git a/src/Magnum/DebugTools/Test/CompareImageTest.cpp b/src/Magnum/DebugTools/Test/CompareImageTest.cpp index 46901efeb..007e275bc 100644 --- a/src/Magnum/DebugTools/Test/CompareImageTest.cpp +++ b/src/Magnum/DebugTools/Test/CompareImageTest.cpp @@ -493,8 +493,9 @@ void CompareImageTest::pixelDeltaSpecials() { void CompareImageTest::compareDifferentSize() { std::stringstream out; - ImageView2D a{PixelFormat::RG8UI, {3, 4}, nullptr}; - ImageView2D b{PixelFormat::RG8UI, {3, 5}, nullptr}; + char data[8*5]; + ImageView2D a{PixelFormat::RG8UI, {3, 4}, data}; + ImageView2D b{PixelFormat::RG8UI, {3, 5}, data}; { TestSuite::Comparator compare{{}, {}}; @@ -511,8 +512,9 @@ void CompareImageTest::compareDifferentSize() { void CompareImageTest::compareDifferentFormat() { std::stringstream out; - ImageView2D a{PixelFormat::RGBA32F, {3, 4}, nullptr}; - ImageView2D b{PixelFormat::RGB32F, {3, 4}, nullptr}; + char data[16*12]; + ImageView2D a{PixelFormat::RGBA32F, {3, 4}, data}; + ImageView2D b{PixelFormat::RGB32F, {3, 4}, data}; { TestSuite::Comparator compare{{}, {}}; diff --git a/src/Magnum/ImageView.cpp b/src/Magnum/ImageView.cpp index be6f319a6..69d6768aa 100644 --- a/src/Magnum/ImageView.cpp +++ b/src/Magnum/ImageView.cpp @@ -35,7 +35,15 @@ template ImageView::ImageView(co template ImageView::ImageView(const PixelStorage storage, const UnsignedInt format, const UnsignedInt formatExtra, const UnsignedInt pixelSize, const VectorTypeFor& size, const Containers::ArrayView data) noexcept: ImageView{storage, pixelFormatWrap(format), formatExtra, pixelSize, size, data} {} template ImageView::ImageView(const PixelStorage storage, const PixelFormat format, const UnsignedInt formatExtra, const UnsignedInt pixelSize, const VectorTypeFor& size, const Containers::ArrayView data) noexcept: _storage{storage}, _format{format}, _formatExtra{formatExtra}, _pixelSize{pixelSize}, _size{size}, _data{reinterpret_cast(data.data()), data.size()} { + #ifdef MAGNUM_BUILD_DEPRECATED + #ifndef CORRADE_NO_ASSERT + if(size.product() && !_data && !_data.size()) + Warning{} << "ImageView::ImageView(): passing empty data to a non-empty view is deprecated, use a constructor without the data parameter instead"; + #endif CORRADE_ASSERT(!_data || Implementation::imageDataSize(*this) <= _data.size(), "ImageView::ImageView(): data too small, got" << _data.size() << "but expected at least" << Implementation::imageDataSize(*this) << "bytes", ); + #else + CORRADE_ASSERT(Implementation::imageDataSize(*this) <= _data.size(), "ImageView::ImageView(): data too small, got" << _data.size() << "but expected at least" << Implementation::imageDataSize(*this) << "bytes", ); + #endif } template ImageView::ImageView(const PixelStorage storage, const PixelFormat format, const VectorTypeFor& size) noexcept: ImageView{storage, format, {}, Magnum::pixelSize(format), size} {} diff --git a/src/Magnum/Test/ImageViewTest.cpp b/src/Magnum/Test/ImageViewTest.cpp index 827a13fcc..ee38c24e9 100644 --- a/src/Magnum/Test/ImageViewTest.cpp +++ b/src/Magnum/Test/ImageViewTest.cpp @@ -39,10 +39,8 @@ struct ImageViewTest: TestSuite::Tester { template void constructGeneric(); template void constructGenericEmpty(); - template void constructGenericEmptyNullptr(); template void constructImplementationSpecific(); template void constructImplementationSpecificEmpty(); - template void constructImplementationSpecificEmptyNullptr(); template void constructCompressedGeneric(); template void constructCompressedGenericEmpty(); template void constructCompressedImplementationSpecific(); @@ -51,6 +49,7 @@ struct ImageViewTest: TestSuite::Tester { void constructFromMutable(); void constructCompressedFromMutable(); + void constructNullptr(); void constructInvalidSize(); void constructCompressedInvalidSize(); @@ -81,14 +80,10 @@ ImageViewTest::ImageViewTest() { &ImageViewTest::constructGeneric, &ImageViewTest::constructGenericEmpty, &ImageViewTest::constructGenericEmpty, - &ImageViewTest::constructGenericEmptyNullptr, - &ImageViewTest::constructGenericEmptyNullptr, &ImageViewTest::constructImplementationSpecific, &ImageViewTest::constructImplementationSpecific, &ImageViewTest::constructImplementationSpecificEmpty, &ImageViewTest::constructImplementationSpecificEmpty, - &ImageViewTest::constructImplementationSpecificEmptyNullptr, - &ImageViewTest::constructImplementationSpecificEmptyNullptr, &ImageViewTest::constructCompressedGeneric, &ImageViewTest::constructCompressedGeneric, &ImageViewTest::constructCompressedGenericEmpty, @@ -101,6 +96,7 @@ ImageViewTest::ImageViewTest() { &ImageViewTest::constructFromMutable, &ImageViewTest::constructCompressedFromMutable, + &ImageViewTest::constructNullptr, &ImageViewTest::constructInvalidSize, &ImageViewTest::constructCompressedInvalidSize, @@ -205,34 +201,6 @@ template void ImageViewTest::constructGenericEmpty() { } } -template void ImageViewTest::constructGenericEmptyNullptr() { - setTestCaseTemplateName(MutabilityTraits::name()); - - /* This should be deprecated/removed, as it doesn't provide anything over - the above and can lead to silent errors */ - - { - ImageView<2, T> a{PixelFormat::RG32F, {2, 6}, nullptr}; - - CORRADE_COMPARE(a.storage().alignment(), 4); - CORRADE_COMPARE(a.format(), PixelFormat::RG32F); - CORRADE_COMPARE(a.formatExtra(), 0); - CORRADE_COMPARE(a.pixelSize(), 8); - CORRADE_COMPARE(a.size(), (Vector2i{2, 6})); - CORRADE_COMPARE(a.data(), nullptr); - } { - ImageView<2, T> a{PixelStorage{}.setAlignment(1), - PixelFormat::RGB16F, {8, 3}, nullptr}; - - CORRADE_COMPARE(a.storage().alignment(), 1); - CORRADE_COMPARE(a.format(), PixelFormat::RGB16F); - CORRADE_COMPARE(a.formatExtra(), 0); - CORRADE_COMPARE(a.pixelSize(), 6); - CORRADE_COMPARE(a.size(), (Vector2i{8, 3})); - CORRADE_COMPARE(a.data(), nullptr); - } -} - template void ImageViewTest::constructImplementationSpecific() { setTestCaseTemplateName(MutabilityTraits::name()); @@ -361,68 +329,6 @@ template void ImageViewTest::constructImplementationSpecificEmpty() { } } -template void ImageViewTest::constructImplementationSpecificEmptyNullptr() { - setTestCaseTemplateName(MutabilityTraits::name()); - - /* This should be deprecated/removed, as it doesn't provide anything over - the above and can lead to silent errors */ - - /* Single format */ - { - ImageView<2, T> a{Vk::PixelFormat::R32G32B32F, {2, 16}, nullptr}; - - CORRADE_COMPARE(a.storage().alignment(), 4); - CORRADE_COMPARE(a.format(), pixelFormatWrap(Vk::PixelFormat::R32G32B32F)); - CORRADE_COMPARE(a.formatExtra(), 0); - CORRADE_COMPARE(a.pixelSize(), 12); - CORRADE_COMPARE(a.size(), (Vector2i{2, 16})); - CORRADE_COMPARE(a.data(), nullptr); - } { - ImageView<2, T> a{PixelStorage{}.setAlignment(1), - Vk::PixelFormat::R32G32B32F, {1, 2}, nullptr}; - - CORRADE_COMPARE(a.storage().alignment(), 1); - CORRADE_COMPARE(a.format(), pixelFormatWrap(Vk::PixelFormat::R32G32B32F)); - CORRADE_COMPARE(a.formatExtra(), 0); - CORRADE_COMPARE(a.pixelSize(), 12); - CORRADE_COMPARE(a.size(), (Vector2i{1, 2})); - CORRADE_COMPARE(a.data(), nullptr); - } - - /* Format + extra */ - { - ImageView<2, T> a{GL::PixelFormat::RGB, GL::PixelType::UnsignedShort, {1, 3}, nullptr}; - - CORRADE_COMPARE(a.storage().alignment(), 4); - CORRADE_COMPARE(a.format(), pixelFormatWrap(GL::PixelFormat::RGB)); - CORRADE_COMPARE(a.formatExtra(), UnsignedInt(GL::PixelType::UnsignedShort)); - CORRADE_COMPARE(a.pixelSize(), 6); - CORRADE_COMPARE(a.size(), (Vector2i{1, 3})); - CORRADE_COMPARE(a.data(), nullptr); - } { - ImageView<2, T> a{PixelStorage{}.setAlignment(1), - GL::PixelFormat::RGB, GL::PixelType::UnsignedShort, {8, 2}, nullptr}; - - CORRADE_COMPARE(a.format(), pixelFormatWrap(GL::PixelFormat::RGB)); - CORRADE_COMPARE(a.formatExtra(), UnsignedInt(GL::PixelType::UnsignedShort)); - CORRADE_COMPARE(a.pixelSize(), 6); - CORRADE_COMPARE(a.size(), (Vector2i{8, 2})); - CORRADE_COMPARE(a.data(), nullptr); - } - - /* Manual pixel size */ - { - ImageView<2, T> a{PixelStorage{}.setAlignment(1), 666, 1337, 6, {3, 3}, nullptr}; - - CORRADE_COMPARE(a.storage().alignment(), 1); - CORRADE_COMPARE(a.format(), pixelFormatWrap(GL::PixelFormat::RGB)); - CORRADE_COMPARE(a.formatExtra(), UnsignedInt(GL::PixelType::UnsignedShort)); - CORRADE_COMPARE(a.pixelSize(), 6); - CORRADE_COMPARE(a.size(), (Vector2i{3, 3})); - CORRADE_COMPARE(a.data(), nullptr); - } -} - template void ImageViewTest::constructCompressedGeneric() { setTestCaseTemplateName(MutabilityTraits::name()); @@ -566,6 +472,17 @@ void ImageViewTest::constructCompressedFromMutable() { CORRADE_COMPARE(b.data().size(), 8); } +void ImageViewTest::constructNullptr() { + #ifdef CORRADE_BUILD_DEPRECATED + CORRADE_SKIP("This is still allowed on a deprecated build, can't test."); + #endif + + std::ostringstream out; + Error redirectError{&out}; + ImageView2D{PixelFormat::RGB8Unorm, {1, 3}, nullptr}; + CORRADE_COMPARE(out.str(), "ImageView::ImageView(): data too small, got 0 but expected at least 12 bytes\n"); +} + void ImageViewTest::constructInvalidSize() { std::ostringstream out; Error redirectError{&out}; diff --git a/src/Magnum/Text/Test/AbstractGlyphCacheTest.cpp b/src/Magnum/Text/Test/AbstractGlyphCacheTest.cpp index 5ad13d1d2..95f1fa272 100644 --- a/src/Magnum/Text/Test/AbstractGlyphCacheTest.cpp +++ b/src/Magnum/Text/Test/AbstractGlyphCacheTest.cpp @@ -127,7 +127,7 @@ void AbstractGlyphCacheTest::setImage() { Vector2i imageOffset, imageSize; } cache{{100, 200}}; - cache.setImage({80, 175}, ImageView2D{{}, {20, 25}, nullptr}); + cache.setImage({80, 175}, ImageView2D{{}, {20, 25}}); CORRADE_COMPARE(cache.imageOffset, (Vector2i{80, 175})); CORRADE_COMPARE(cache.imageSize, (Vector2i{20, 25})); @@ -138,9 +138,9 @@ void AbstractGlyphCacheTest::setImageOutOfBounds() { std::ostringstream out; Error redirectError{&out}; - cache.setImage({80, 175}, ImageView2D{{}, {20, 25}, nullptr}); - cache.setImage({81, 175}, ImageView2D{{}, {20, 25}, nullptr}); - cache.setImage({80, -1}, ImageView2D{{}, {20, 25}, nullptr}); + cache.setImage({80, 175}, ImageView2D{{}, {20, 25}}); + cache.setImage({81, 175}, ImageView2D{{}, {20, 25}}); + cache.setImage({80, -1}, ImageView2D{{}, {20, 25}}); CORRADE_COMPARE(out.str(), "Text::AbstractGlyphCache::setImage(): Range({81, 175}, {101, 200}) out of bounds for texture size Vector(100, 200)\n" diff --git a/src/Magnum/Trade/Test/AbstractImageConverterTest.cpp b/src/Magnum/Trade/Test/AbstractImageConverterTest.cpp index 6a445e7d3..799d16d26 100644 --- a/src/Magnum/Trade/Test/AbstractImageConverterTest.cpp +++ b/src/Magnum/Trade/Test/AbstractImageConverterTest.cpp @@ -188,7 +188,7 @@ void AbstractImageConverterTest::exportToImageNotImplemented() { Error redirectError{&out}; Converter converter; - converter.exportToImage(ImageView2D{PixelFormat::R8Unorm, {4, 6}, Containers::ArrayView{nullptr, 24}}); + converter.exportToImage(ImageView2D{PixelFormat::R8Unorm, {4, 6}, Containers::ArrayView{nullptr, 128}}); CORRADE_COMPARE(out.str(), "Trade::AbstractImageConverter::exportToImage(): feature advertised but not implemented\n"); } @@ -201,7 +201,7 @@ void AbstractImageConverterTest::exportToCompressedImage() { }; Converter converter; - Containers::Optional actual = converter.exportToCompressedImage(ImageView2D{PixelFormat::R8Unorm, {16, 8}, Containers::ArrayView{nullptr, 24}}); + Containers::Optional actual = converter.exportToCompressedImage(ImageView2D{PixelFormat::R8Unorm, {16, 8}, Containers::ArrayView{nullptr, 128}}); CORRADE_VERIFY(actual); CORRADE_COMPARE(actual->data().size(), 64); CORRADE_COMPARE(actual->size(), (Vector2i{16, 8})); @@ -216,7 +216,7 @@ void AbstractImageConverterTest::exportToCompressedImageNotSupported() { Error redirectError{&out}; Converter converter; - converter.exportToCompressedImage(ImageView2D{PixelFormat::R8Unorm, {16, 8}, Containers::ArrayView{nullptr, 24}}); + converter.exportToCompressedImage(ImageView2D{PixelFormat::R8Unorm, {16, 8}, Containers::ArrayView{nullptr, 128}}); CORRADE_COMPARE(out.str(), "Trade::AbstractImageConverter::exportToCompressedImage(): feature not supported\n"); } @@ -229,7 +229,7 @@ void AbstractImageConverterTest::exportToCompressedImageNotImplemented() { Error redirectError{&out}; Converter converter; - converter.exportToCompressedImage(ImageView2D{PixelFormat::R8Unorm, {16, 8}, Containers::ArrayView{nullptr, 24}}); + converter.exportToCompressedImage(ImageView2D{PixelFormat::R8Unorm, {16, 8}, Containers::ArrayView{nullptr, 128}}); CORRADE_COMPARE(out.str(), "Trade::AbstractImageConverter::exportToCompressedImage(): feature advertised but not implemented\n"); }