ic
/
magnum
mirror of https://github.com/mosra/magnum.git
6
0
Fork 0
Code Issues Projects Releases Wiki Activity
Browse Source

Trade: the SceneFieldData string constructors can't be constexpr. 

Apparently I forgot to actually test these -- in order to fit the string
data pointer without making SceneFieldData too large, it'đ stored as an
offset from fieldData. And that's something not expressible in a
constexpr context. Thus the only way to create a constexpr string field
is by using the offset-only constructor (which is now appropriately
tested).

This change at least allowed me to move the constructor to the cpp file,
saving on header size and using more lightweight assertions.
pull/601/head
Vladimír Vondruš 3 years ago
parent
769cc9ee2a
commit
cb5e8f4569
3 changed files with 73 additions and 25 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 30
      src/Magnum/Trade/SceneData.cpp
  2. 36
      src/Magnum/Trade/SceneData.h
  3. 32
      src/Magnum/Trade/Test/SceneDataTest.cpp

30
src/Magnum/Trade/SceneData.cpp
Unescape View File

@ -575,6 +575,36 @@ SceneFieldData::SceneFieldData(const SceneField name, const Containers::StridedA
CORRADE_ASSERT(fieldData.isContiguous<1>(), "Trade::SceneFieldData: second field view dimension is not contiguous", );
}
SceneFieldData::SceneFieldData(const SceneField name, const SceneMappingType mappingType, const Containers::StridedArrayView1D<const void>& mappingData, const char* const stringData, const SceneFieldType fieldType, const Containers::StridedArrayView1D<const void>& fieldData, const SceneFieldFlags flags) noexcept:
_size{mappingData.size()},
_name{name},
_flags{flags|Implementation::implicitSceneFieldFlagsFor(fieldType)},
_mappingTypeStringType{UnsignedByte(UnsignedByte(mappingType)|(UnsignedShort(fieldType) << 3))},
_mappingStride{Short(mappingData.stride())},
_mappingData{mappingData.data()},
_field{Short(fieldData.stride()),
/* This expression is the reason why the constructor can't be constexpr
-- the only possibility for this to work would be if
fieldData.data() would give back a const char* pointer to avoid the
cast (which is dangerous on its own, as the data is inherently
sparse), and even then it'd probably fail due to the two pointers
being two unrelated pieces of memory */
stringData - static_cast<const char*>(fieldData.data())},
_fieldData{fieldData.data()} {
CORRADE_ASSERT(mappingData.size() == fieldData.size(),
"Trade::SceneFieldData: expected" << name << "mapping and field view to have the same size but got" << mappingData.size() << "and" << fieldData.size(), );
CORRADE_ASSERT(Implementation::isSceneFieldTypeCompatibleWithField(name, fieldType),
"Trade::SceneFieldData:" << fieldType << "is not a valid type for" << name, );
CORRADE_ASSERT(!(flags & SceneFieldFlag::OffsetOnly),
"Trade::SceneFieldData: can't pass" << (flags & SceneFieldFlag::OffsetOnly) << "for a view", );
CORRADE_ASSERT(Implementation::isSceneFieldTypeString(fieldType),
"Trade::SceneFieldData: can't use a string constructor for" << fieldType, );
CORRADE_ASSERT(mappingData.stride() >= -32768 && mappingData.stride() <= 32767,
"Trade::SceneFieldData: expected mapping view stride to fit into 16 bits but got" << mappingData.stride(), );
CORRADE_ASSERT(fieldData.stride() >= -32768 && fieldData.stride() <= 32767,
"Trade::SceneFieldData: expected field view stride to fit into 16 bits but got" << fieldData.stride(), );
}
SceneFieldData::SceneFieldData(const SceneField name, const Containers::StridedArrayView2D<const char>& mappingData, const char* const stringData, const SceneFieldType fieldType, const Containers::StridedArrayView2D<const char>& fieldData, const SceneFieldFlags flags) noexcept: SceneFieldData{name, {}, Containers::StridedArrayView1D<const void>{{mappingData.data(), ~std::size_t{}}, mappingData.size()[0], mappingData.stride()[0]}, stringData, fieldType, Containers::StridedArrayView1D<const void>{{fieldData.data(), ~std::size_t{}}, fieldData.size()[0], fieldData.stride()[0]}, flags} {
/* Yes, this calls into a constexpr function defined in the header --
because I feel that makes more sense than duplicating the full assert

36
src/Magnum/Trade/SceneData.h
Unescape View File

@ -986,8 +986,13 @@ class MAGNUM_TRADE_EXPORT SceneFieldData {
checking happens in the SceneField constructor, at which point the
size would be gone anyway as SceneFieldData can store only the begin
pointer inside. Using it would also mean we'd need to include its
full definition in this header. */
constexpr explicit SceneFieldData(SceneField name, SceneMappingType mappingType, const Containers::StridedArrayView1D<const void>& mappingData, const char* stringData, SceneFieldType fieldType, const Containers::StridedArrayView1D<const void>& fieldData, SceneFieldFlags flags = {}) noexcept;
full definition in this header.
Not constexpr because internally we store the 48-bit
`stringData - fieldData` offset, which is not possible to do in a
constexpr context. Only the offset-only constructor is usable for
creating constexpr string fields. */
explicit SceneFieldData(SceneField name, SceneMappingType mappingType, const Containers::StridedArrayView1D<const void>& mappingData, const char* stringData, SceneFieldType fieldType, const Containers::StridedArrayView1D<const void>& fieldData, SceneFieldFlags flags = {}) noexcept;
/**
* @brief Construct a string field from 2D type-erased views
@ -1034,11 +1039,12 @@ class MAGNUM_TRADE_EXPORT SceneFieldData {
* @relativeref{SceneFieldType,StringRangeNullTerminated32} and
* @relativeref{SceneFieldType,StringRangeNullTerminated64}.
*/
/* See above for why const char* is used instead of StringView */
template<class T> constexpr explicit SceneFieldData(SceneField name, const Containers::StridedArrayView1D<T>& mappingData, const char* stringData, SceneFieldType fieldType, const Containers::StridedArrayView1D<const void>& fieldData, SceneFieldFlags flags = {}) noexcept;
/* See above for why const char* is used instead of StringView and why
this function is not constexpr */
template<class T> explicit SceneFieldData(SceneField name, const Containers::StridedArrayView1D<T>& mappingData, const char* stringData, SceneFieldType fieldType, const Containers::StridedArrayView1D<const void>& fieldData, SceneFieldFlags flags = {}) noexcept;
/** @overload */
template<class T> constexpr explicit SceneFieldData(SceneField name, const Containers::ArrayView<T>& mappingData, const char* stringData, SceneFieldType fieldType, const Containers::StridedArrayView1D<const void>& fieldData, SceneFieldFlags flags = {}) noexcept: SceneFieldData{name, Containers::stridedArrayView(mappingData), stringData, fieldType, fieldData, flags} {}
template<class T> explicit SceneFieldData(SceneField name, const Containers::ArrayView<T>& mappingData, const char* stringData, SceneFieldType fieldType, const Containers::StridedArrayView1D<const void>& fieldData, SceneFieldFlags flags = {}) noexcept: SceneFieldData{name, Containers::stridedArrayView(mappingData), stringData, fieldType, fieldData, flags} {}
/**
* @brief Construct an offset-only field
@ -3684,25 +3690,7 @@ template<class T, class U> constexpr SceneFieldData::SceneFieldData(const SceneF
flags
} {}
constexpr SceneFieldData::SceneFieldData(const SceneField name, const SceneMappingType mappingType, const Containers::StridedArrayView1D<const void>& mappingData, const char* const stringData, const SceneFieldType fieldType, const Containers::StridedArrayView1D<const void>& fieldData, const SceneFieldFlags flags) noexcept:
_size{(CORRADE_CONSTEXPR_ASSERT(mappingData.size() == fieldData.size(),
"Trade::SceneFieldData: expected" << name << "mapping and field view to have the same size but got" << mappingData.size() << "and" << fieldData.size()), mappingData.size())},
_name{(CORRADE_CONSTEXPR_ASSERT(Implementation::isSceneFieldTypeCompatibleWithField(name, fieldType),
"Trade::SceneFieldData:" << fieldType << "is not a valid type for" << name), name)},
_flags{(CORRADE_CONSTEXPR_ASSERT(!(flags & SceneFieldFlag::OffsetOnly),
"Trade::SceneFieldData: can't pass" << (flags & SceneFieldFlag::OffsetOnly) << "for a view"), flags|Implementation::implicitSceneFieldFlagsFor(fieldType))},
_mappingTypeStringType{(CORRADE_CONSTEXPR_ASSERT(Implementation::isSceneFieldTypeString(fieldType),
"Trade::SceneFieldData: can't use a string constructor for" << fieldType), UnsignedByte(UnsignedByte(mappingType)|(UnsignedShort(fieldType) << 3)))},
_mappingStride{(CORRADE_CONSTEXPR_ASSERT(mappingData.stride() >= -32768 && mappingData.stride() <= 32767,
"Trade::SceneFieldData: expected mapping view stride to fit into 16 bits but got" << mappingData.stride()), Short(mappingData.stride()))},
_mappingData{mappingData.data()},
_field{
(CORRADE_CONSTEXPR_ASSERT(fieldData.stride() >= -32768 && fieldData.stride() <= 32767,
"Trade::SceneFieldData: expected field view stride to fit into 16 bits but got" << fieldData.stride()), Short(fieldData.stride())),
stringData - static_cast<const char*>(fieldData.data())},
_fieldData{fieldData.data()} {}
template<class T> constexpr SceneFieldData::SceneFieldData(const SceneField name, const Containers::StridedArrayView1D<T>& mappingData, const char* stringData, SceneFieldType fieldType, const Containers::StridedArrayView1D<const void>& fieldData, const SceneFieldFlags flags) noexcept: SceneFieldData{name, Implementation::sceneMappingTypeFor<typename std::remove_const<T>::type>(), mappingData, stringData, fieldType, fieldData, flags} {}
template<class T> inline SceneFieldData::SceneFieldData(const SceneField name, const Containers::StridedArrayView1D<T>& mappingData, const char* stringData, SceneFieldType fieldType, const Containers::StridedArrayView1D<const void>& fieldData, const SceneFieldFlags flags) noexcept: SceneFieldData{name, Implementation::sceneMappingTypeFor<typename std::remove_const<T>::type>(), mappingData, stringData, fieldType, fieldData, flags} {}
constexpr SceneFieldData::SceneFieldData(const SceneField name, const std::size_t size, const SceneMappingType mappingType, const std::size_t mappingOffset, const std::ptrdiff_t mappingStride, const SceneFieldType fieldType, const std::size_t fieldOffset, const std::ptrdiff_t fieldStride, const UnsignedShort fieldArraySize, const SceneFieldFlags flags) noexcept:
_size{size},

32
src/Magnum/Trade/Test/SceneDataTest.cpp
Unescape View File

@ -852,7 +852,7 @@ void SceneDataTest::constructField() {
CORRADE_COMPARE(flags, SceneFieldFlag::ImplicitMapping);
CORRADE_COMPARE(mappingType, SceneMappingType::UnsignedShort);
/* These are not marked constexpr because it'd work only partially, not for
string fields (tested in constructFieldString()) */
string fields (tested in constructFieldOffsetOnlyString()) */
CORRADE_COMPARE(crotations.fieldType(), SceneFieldType::Complexd);
CORRADE_COMPARE(crotations.fieldArraySize(), 0);
/* These are deliberately not constexpr to save header size a bit --
@ -931,6 +931,9 @@ void SceneDataTest::constructFieldString() {
CORRADE_COMPARE(names.fieldData(someArray).stride(), sizeof(Containers::Pair<UnsignedShort, UnsignedShort>));
CORRADE_COMPARE(names.fieldData(someArray).data(), data.nameField);
CORRADE_COMPARE(names.stringData(someArray), static_cast<const void*>(data.nameString));
/* Construction of a string field is not constexpr due to arithmetic on two
(differently cast) pointers */
}
void SceneDataTest::constructFieldStringNegativeStride() {
@ -1033,6 +1036,9 @@ void SceneDataTest::constructFieldTypeErasedString() {
CORRADE_COMPARE(names.fieldData().stride(), sizeof(UnsignedShort)*2);
CORRADE_COMPARE(names.fieldData().data(), nameFieldData);
CORRADE_COMPARE(names.stringData(), static_cast<const void*>(nameStringData));
/* Construction of a string field is not constexpr due to arithmetic on two
(differently cast) pointers */
}
void SceneDataTest::constructFieldTypeErased2D() {
@ -1148,6 +1154,30 @@ void SceneDataTest::constructFieldOffsetOnlyString() {
CORRADE_COMPARE(a.stringData(string), "eyehandnoseleg"_s);
CORRADE_COMPARE((Containers::StringView{a.stringData(string) + fieldData[0].first(), fieldData[0].second()}), "hand");
CORRADE_COMPARE((Containers::StringView{a.stringData(string) + fieldData[1].first(), fieldData[1].second()}), "leg");
constexpr SceneFieldData ca{sceneFieldCustom(36), 2, SceneMappingType::UnsignedLong, offsetof(Data, object), sizeof(Data), 6, SceneFieldType::StringRange8, offsetof(Data, nameRange), sizeof(Data), SceneFieldFlag::OrderedMapping};
constexpr SceneField name = ca.name();
constexpr SceneFieldFlags flags = ca.flags();
constexpr std::size_t size = ca.size();
constexpr SceneMappingType mappingType = ca.mappingType();
CORRADE_COMPARE(name, sceneFieldCustom(36));
CORRADE_COMPARE(flags, SceneFieldFlag::OffsetOnly|SceneFieldFlag::OrderedMapping);
CORRADE_COMPARE(size, 2);
CORRADE_COMPARE(mappingType, SceneMappingType::UnsignedLong);
/* These are not marked constexpr because it wouldn't work for string
fields due to the type/size stored in an union */
CORRADE_COMPARE(ca.fieldType(), SceneFieldType::StringRange8);
CORRADE_COMPARE(ca.fieldArraySize(), 0);
/* These are deliberately not constexpr to save header size a bit --
compared to SceneField APIs they get used very little and it's mostly
useless in a constexpr context anyway */
CORRADE_COMPARE(ca.mappingData(data).size(), 2);
CORRADE_COMPARE(ca.mappingData(data).stride(), sizeof(Data));
CORRADE_COMPARE_AS(Containers::arrayCast<const UnsignedLong>(ca.mappingData(data)),
Containers::arrayView<UnsignedLong>({2, 15}),
TestSuite::Compare::Container);
CORRADE_COMPARE(ca.fieldData(data).size(), 2);
CORRADE_COMPARE(ca.fieldData(data).stride(), sizeof(Data));
}
void SceneDataTest::constructFieldOffsetOnlyStringNegativeStride() {

Write Preview
Loading…
Cancel
Save