From e08efa62a1305ec7d69bee1c7b4ed99bbd8144a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Tue, 14 Dec 2021 11:58:42 +0100 Subject: [PATCH] Trade: disallow empty MaterialAttributeData names. Overlooked during the initial design -- it'd sort before " LayerName", which is undesirable. --- src/Magnum/Trade/MaterialData.cpp | 3 ++ src/Magnum/Trade/MaterialData.h | 16 ++++++--- src/Magnum/Trade/Test/MaterialDataTest.cpp | 40 ++++++++++++++++++++++ 3 files changed, 54 insertions(+), 5 deletions(-) diff --git a/src/Magnum/Trade/MaterialData.cpp b/src/Magnum/Trade/MaterialData.cpp index bcff89b2b..3ea199711 100644 --- a/src/Magnum/Trade/MaterialData.cpp +++ b/src/Magnum/Trade/MaterialData.cpp @@ -130,6 +130,9 @@ std::size_t materialAttributeTypeSize(const MaterialAttributeType type) { } MaterialAttributeData::MaterialAttributeData(const Containers::StringView name, const MaterialAttributeType type, const std::size_t size, const void* const value) noexcept: _data{} /* zero-initialized */ { + /* It would sort before " LayerName" and that's not desirable */ + CORRADE_ASSERT(!name.isEmpty(), "Trade::MaterialAttributeData: name is not allowed to be empty", ); + /* Special handling for strings */ if(type == MaterialAttributeType::String) { const auto& stringValue = *static_cast(value); diff --git a/src/Magnum/Trade/MaterialData.h b/src/Magnum/Trade/MaterialData.h index 3debd0bec..754758720 100644 --- a/src/Magnum/Trade/MaterialData.h +++ b/src/Magnum/Trade/MaterialData.h @@ -2906,20 +2906,26 @@ template constexpr MaterialAttributeData::MaterialAttributeData(const Containers::StringView name, const T& value) noexcept: - _data{Implementation::MaterialAttributeTypeFor::type(), + _data{Implementation::MaterialAttributeTypeFor::type(), ( + /* It would sort before " LayerName" and that's not desirable */ + CORRADE_CONSTEXPR_ASSERT(!name.isEmpty(), "Trade::MaterialAttributeData: name is not allowed to be empty"), /* MSVC 2015 complains about "error C2065: 'T': undeclared identifier" in the lambda inside this macro. Sorry, the assert will be less useful on that stupid thing. */ #ifndef CORRADE_MSVC2015_COMPATIBILITY - (CORRADE_CONSTEXPR_ASSERT(name.size() + sizeof(T) + 2 <= Implementation::MaterialAttributeDataSize, "Trade::MaterialAttributeData: name" << name << "too long, expected at most" << Implementation::MaterialAttributeDataSize - sizeof(T) - 2 << "bytes for" << Implementation::MaterialAttributeTypeFor::type() << "but got" << name.size()), name) + CORRADE_CONSTEXPR_ASSERT(name.size() + sizeof(T) + 2 <= Implementation::MaterialAttributeDataSize, "Trade::MaterialAttributeData: name" << name << "too long, expected at most" << Implementation::MaterialAttributeDataSize - sizeof(T) - 2 << "bytes for" << Implementation::MaterialAttributeTypeFor::type() << "but got" << name.size()), #else - (CORRADE_CONSTEXPR_ASSERT(name.size() + sizeof(T) + 2 <= Implementation::MaterialAttributeDataSize, "Trade::MaterialAttributeData: name" << name << "too long, got" << name.size() << "bytes"), name) + CORRADE_CONSTEXPR_ASSERT(name.size() + sizeof(T) + 2 <= Implementation::MaterialAttributeDataSize, "Trade::MaterialAttributeData: name" << name << "too long, got" << name.size() << "bytes"), #endif - , value} {} + name), value} {} /* The 4 extra bytes are for a null byte after both the name and value, a type and a string size */ -constexpr MaterialAttributeData::MaterialAttributeData(const Containers::StringView name, const Containers::StringView value) noexcept: _data{(CORRADE_CONSTEXPR_ASSERT(name.size() + value.size() + 4 <= Implementation::MaterialAttributeDataSize, "Trade::MaterialAttributeData: name" << name << "and value" << value << "too long, expected at most" << Implementation::MaterialAttributeDataSize - 4 << "bytes in total but got" << name.size() + value.size()), name), value} {} +constexpr MaterialAttributeData::MaterialAttributeData(const Containers::StringView name, const Containers::StringView value) noexcept: _data{( + /* It would sort before " LayerName" and that's not desirable */ + CORRADE_CONSTEXPR_ASSERT(!name.isEmpty(), "Trade::MaterialAttributeData: name is not allowed to be empty"), + CORRADE_CONSTEXPR_ASSERT(name.size() + value.size() + 4 <= Implementation::MaterialAttributeDataSize, "Trade::MaterialAttributeData: name" << name << "and value" << value << "too long, expected at most" << Implementation::MaterialAttributeDataSize - 4 << "bytes in total but got" << name.size() + value.size()), + name), value} {} template T MaterialAttributeData::value() const { CORRADE_ASSERT(Implementation::MaterialAttributeTypeFor::type() == _data.type, diff --git a/src/Magnum/Trade/Test/MaterialDataTest.cpp b/src/Magnum/Trade/Test/MaterialDataTest.cpp index 54aa9369c..22c7879b0 100644 --- a/src/Magnum/Trade/Test/MaterialDataTest.cpp +++ b/src/Magnum/Trade/Test/MaterialDataTest.cpp @@ -73,6 +73,8 @@ class MaterialDataTest: public TestSuite::Tester { void constructAttributeInvalidLayerName(); void constructAttributeWrongTypeForName(); void constructAttributeInvalidType(); + void constructAttributeEmptyName(); + void constructAttributeEmptyNameString(); void constructAttributeTooLarge(); void constructAttributeTooLargeString(); void constructAttributeTooLargeNameString(); @@ -214,6 +216,8 @@ MaterialDataTest::MaterialDataTest() { &MaterialDataTest::constructAttributeInvalidLayerName, &MaterialDataTest::constructAttributeWrongTypeForName, &MaterialDataTest::constructAttributeInvalidType, + &MaterialDataTest::constructAttributeEmptyName, + &MaterialDataTest::constructAttributeEmptyNameString, &MaterialDataTest::constructAttributeTooLarge, &MaterialDataTest::constructAttributeTooLargeString, &MaterialDataTest::constructAttributeTooLargeNameString, @@ -707,6 +711,42 @@ void MaterialDataTest::constructAttributeInvalidType() { "Trade::materialAttributeTypeSize(): invalid type Trade::MaterialAttributeType(0xfe)\n"); } +void MaterialDataTest::constructAttributeEmptyName() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + std::ostringstream out; + Error redirectError{&out}; + MaterialAttributeData{"", Int{}}; + /* Constexpr variant has the same assert, but in the header. It should have + the same output. */ + /*constexpr*/ MaterialAttributeData{""_s, Int{}}; + CORRADE_COMPARE(out.str(), + "Trade::MaterialAttributeData: name is not allowed to be empty\n" + "Trade::MaterialAttributeData: name is not allowed to be empty\n"); +} + +void MaterialDataTest::constructAttributeEmptyNameString() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + /* This has no reason to not be allowed */ + MaterialAttributeData{"hello this string is empty", ""}; + MaterialAttributeData{"hello this string is empty"_s, ""_s}; + + std::ostringstream out; + Error redirectError{&out}; + MaterialAttributeData{"", "hello"}; + /* Constexpr variant has the same assert, but in the header. It should have + the same output. */ + /*constexpr*/ MaterialAttributeData{""_s, "hello"_s}; + CORRADE_COMPARE(out.str(), + "Trade::MaterialAttributeData: name is not allowed to be empty\n" + "Trade::MaterialAttributeData: name is not allowed to be empty\n"); +} + void MaterialDataTest::constructAttributeTooLarge() { #ifdef CORRADE_NO_ASSERT CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions");