From 414e80bbe29bab0ee7ff9cfdea6d0d2458a0e1e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sun, 18 Jun 2023 23:02:49 +0200 Subject: [PATCH] Trade: avoid double lookup in template MeshData and SceneData accessors. The second lookup was only needed for a type compatibility assertion, which is a bit unfortunate. It wouldn't be a problem on a no-assert build, but for various reasons people shouldn't be using these. Too dangerous. --- src/Magnum/Trade/MeshData.h | 38 +++++++++++++++++++++++------------- src/Magnum/Trade/SceneData.h | 28 ++++++++++++++++++-------- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/src/Magnum/Trade/MeshData.h b/src/Magnum/Trade/MeshData.h index 76c54e7f0..54ea5d38c 100644 --- a/src/Magnum/Trade/MeshData.h +++ b/src/Magnum/Trade/MeshData.h @@ -2598,45 +2598,55 @@ template Containers::StridedArrayView2D Containers::StridedArrayView1D MeshData::attribute(const MeshAttribute name, const UnsignedInt id) const { - Containers::StridedArrayView2D data = attribute(name, id); - #ifdef CORRADE_GRACEFUL_ASSERT /* Sigh. Brittle. Better idea? */ - if(!data.stride()[1]) return {}; - #endif + const UnsignedInt attributeId = findAttributeIdInternal(name, id); + CORRADE_ASSERT(attributeId != ~UnsignedInt{}, + "Trade::MeshData::attribute(): index" << id << "out of range for" << attributeCount(name) << name << "attributes", {}); + const Containers::StridedArrayView2D data = attribute(attributeId); + /* Unlike mutableAttribute(), the above can't fail, so no early return with + CORRADE_GRACEFUL_ASSERT */ #ifndef CORRADE_NO_ASSERT - if(!checkVertexFormatCompatibility(_attributes[findAttributeIdInternal(name, id)], "Trade::MeshData::attribute():")) return {}; + if(!checkVertexFormatCompatibility(_attributes[attributeId], "Trade::MeshData::attribute():")) return {}; #endif return Containers::arrayCast<1, const T>(data); } template Containers::StridedArrayView2D::type> MeshData::attribute(const MeshAttribute name, const UnsignedInt id) const { - Containers::StridedArrayView2D data = attribute(name, id); - #ifdef CORRADE_GRACEFUL_ASSERT /* Sigh. Brittle. Better idea? */ - if(!data.stride()[1]) return {}; - #endif + const UnsignedInt attributeId = findAttributeIdInternal(name, id); + CORRADE_ASSERT(attributeId != ~UnsignedInt{}, + "Trade::MeshData::attribute(): index" << id << "out of range for" << attributeCount(name) << name << "attributes", {}); + const Containers::StridedArrayView2D data = attribute(attributeId); + /* Unlike mutableAttribute(), the above can't fail, so no early return with + CORRADE_GRACEFUL_ASSERT */ #ifndef CORRADE_NO_ASSERT - if(!checkVertexFormatCompatibility(_attributes[findAttributeIdInternal(name, id)], "Trade::MeshData::attribute():")) return {}; + if(!checkVertexFormatCompatibility(_attributes[attributeId], "Trade::MeshData::attribute():")) return {}; #endif return Containers::arrayCast<2, const typename std::remove_extent::type>(data); } template Containers::StridedArrayView1D MeshData::mutableAttribute(const MeshAttribute name, const UnsignedInt id) { - Containers::StridedArrayView2D data = mutableAttribute(name, id); + const UnsignedInt attributeId = findAttributeIdInternal(name, id); + CORRADE_ASSERT(attributeId != ~UnsignedInt{}, + "Trade::MeshData::mutableAttribute(): index" << id << "out of range for" << attributeCount(name) << name << "attributes", {}); + Containers::StridedArrayView2D data = mutableAttribute(attributeId); #ifdef CORRADE_GRACEFUL_ASSERT /* Sigh. Brittle. Better idea? */ if(!data.stride()[1]) return {}; #endif #ifndef CORRADE_NO_ASSERT - if(!checkVertexFormatCompatibility(_attributes[findAttributeIdInternal(name, id)], "Trade::MeshData::mutableAttribute():")) return {}; + if(!checkVertexFormatCompatibility(_attributes[attributeId], "Trade::MeshData::mutableAttribute():")) return {}; #endif return Containers::arrayCast<1, T>(data); } template Containers::StridedArrayView2D::type> MeshData::mutableAttribute(const MeshAttribute name, const UnsignedInt id) { - Containers::StridedArrayView2D data = mutableAttribute(name, id); + const UnsignedInt attributeId = findAttributeIdInternal(name, id); + CORRADE_ASSERT(attributeId != ~UnsignedInt{}, + "Trade::MeshData::mutableAttribute(): index" << id << "out of range for" << attributeCount(name) << name << "attributes", {}); + Containers::StridedArrayView2D data = mutableAttribute(attributeId); #ifdef CORRADE_GRACEFUL_ASSERT /* Sigh. Brittle. Better idea? */ if(!data.stride()[1]) return {}; #endif #ifndef CORRADE_NO_ASSERT - if(!checkVertexFormatCompatibility(_attributes[findAttributeIdInternal(name, id)], "Trade::MeshData::mutableAttribute():")) return {}; + if(!checkVertexFormatCompatibility(_attributes[attributeId], "Trade::MeshData::mutableAttribute():")) return {}; #endif return Containers::arrayCast<2, typename std::remove_extent::type>(data); } diff --git a/src/Magnum/Trade/SceneData.h b/src/Magnum/Trade/SceneData.h index c4bb3ffd5..c645f8791 100644 --- a/src/Magnum/Trade/SceneData.h +++ b/src/Magnum/Trade/SceneData.h @@ -4295,45 +4295,57 @@ template Containers::StridedArrayView2D Containers::StridedArrayView1D SceneData::field(const SceneField name) const { - Containers::StridedArrayView2D data = field(name); + const UnsignedInt fieldId = findFieldIdInternal(name); + CORRADE_ASSERT(fieldId != ~UnsignedInt{}, + "Trade::SceneData::field(): field" << name << "not found", {}); + Containers::StridedArrayView2D data = field(fieldId); #ifdef CORRADE_GRACEFUL_ASSERT /* Sigh. Brittle. Better idea? */ if(!data.stride()[1]) return {}; #endif #ifndef CORRADE_NO_ASSERT - if(!checkFieldTypeCompatibility(_fields[findFieldIdInternal(name)], "Trade::SceneData::field():")) return {}; + if(!checkFieldTypeCompatibility(_fields[fieldId], "Trade::SceneData::field():")) return {}; #endif return Containers::arrayCast<1, const T>(data); } template Containers::StridedArrayView2D::type> SceneData::field(const SceneField name) const { - Containers::StridedArrayView2D data = field(name); + const UnsignedInt fieldId = findFieldIdInternal(name); + CORRADE_ASSERT(fieldId != ~UnsignedInt{}, + "Trade::SceneData::field(): field" << name << "not found", {}); + Containers::StridedArrayView2D data = field(fieldId); #ifdef CORRADE_GRACEFUL_ASSERT /* Sigh. Brittle. Better idea? */ if(!data.stride()[1]) return {}; #endif #ifndef CORRADE_NO_ASSERT - if(!checkFieldTypeCompatibility(_fields[findFieldIdInternal(name)], "Trade::SceneData::field():")) return {}; + if(!checkFieldTypeCompatibility(_fields[fieldId], "Trade::SceneData::field():")) return {}; #endif return Containers::arrayCast<2, const typename std::remove_extent::type>(data); } template Containers::StridedArrayView1D SceneData::mutableField(const SceneField name) { - Containers::StridedArrayView2D data = mutableField(name); + const UnsignedInt fieldId = findFieldIdInternal(name); + CORRADE_ASSERT(fieldId != ~UnsignedInt{}, + "Trade::SceneData::mutableField(): field" << name << "not found", {}); + Containers::StridedArrayView2D data = mutableField(fieldId); #ifdef CORRADE_GRACEFUL_ASSERT /* Sigh. Brittle. Better idea? */ if(!data.stride()[1]) return {}; #endif #ifndef CORRADE_NO_ASSERT - if(!checkFieldTypeCompatibility(_fields[findFieldIdInternal(name)], "Trade::SceneData::mutableField():")) return {}; + if(!checkFieldTypeCompatibility(_fields[fieldId], "Trade::SceneData::mutableField():")) return {}; #endif return Containers::arrayCast<1, T>(data); } template Containers::StridedArrayView2D::type> SceneData::mutableField(const SceneField name) { - Containers::StridedArrayView2D data = mutableField(name); + const UnsignedInt fieldId = findFieldIdInternal(name); + CORRADE_ASSERT(fieldId != ~UnsignedInt{}, + "Trade::SceneData::mutableField(): field" << name << "not found", {}); + Containers::StridedArrayView2D data = mutableField(fieldId); #ifdef CORRADE_GRACEFUL_ASSERT /* Sigh. Brittle. Better idea? */ if(!data.stride()[1]) return {}; #endif #ifndef CORRADE_NO_ASSERT - if(!checkFieldTypeCompatibility(_fields[findFieldIdInternal(name)], "Trade::SceneData::mutableField():")) return {}; + if(!checkFieldTypeCompatibility(_fields[fieldId], "Trade::SceneData::mutableField():")) return {}; #endif return Containers::arrayCast<2, typename std::remove_extent::type>(data); }