From 12981363a71f5155958cfbc48532c46f805948a9 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 23 May 2023 09:00:52 -0400 Subject: [PATCH] Be a little more flexible about Span constructors. (#26701) Before this change, this code: struct X { int member; }; struct Y : public X { }; Y object; Span span(&Y, 1); would not compile, because we had std::is_same checks on the types (X and Y in this case). But this code is in fact safe as long as sizeof(Y) == sizeof(X), so we don't get confused about our object boundaries. This change replaces the std::is_same checks with sizeof() checks. But that leads to some situations being ambiguous, because the "can this pointer actually work for us?" check happens after overload resolution, so we explicitly do the std::is_convertible checks alongside sizeof to make sure that the pointer passed to our Span constructor will work. --- src/lib/support/Span.h | 39 ++++++++++++++------------- src/lib/support/tests/TestSpan.cpp | 42 +++++++++++++++++++++++++++--- 2 files changed, 59 insertions(+), 22 deletions(-) diff --git a/src/lib/support/Span.h b/src/lib/support/Span.h index 854ee34d1a410f..61c375b85041f3 100644 --- a/src/lib/support/Span.h +++ b/src/lib/support/Span.h @@ -43,11 +43,11 @@ class Span constexpr Span() : mDataBuf(nullptr), mDataLen(0) {} constexpr Span(pointer databuf, size_t datalen) : mDataBuf(databuf), mDataLen(datalen) {} - template - constexpr explicit Span(T (&databuf)[N]) : Span(databuf, N) + template ::value>> + constexpr explicit Span(U (&databuf)[N]) : Span(databuf, N) {} - template , std::remove_const_t>::value>> + template ::value>> constexpr Span(std::array & arr) : mDataBuf(arr.data()), mDataLen(N) {} @@ -60,14 +60,20 @@ class Span } // Allow implicit construction from a Span over a type that matches our - // type, up to const-ness. - template , std::remove_const_t>::value>> + // type's size, if a pointer to the other type can be treated as a pointer + // to our type (e.g. other type is same as ours, or is a same-size + // subclass). The size check is really important to make sure we don't get + // confused about where our object boundaries are. + template ::value>> constexpr Span(const Span & other) : Span(other.data(), other.size()) {} // Allow implicit construction from a FixedSpan over a type that matches our - // type, up to const-ness. - template , std::remove_const_t>::value>> + // type's size, if a pointer to the other type can be treated as a pointer + // to our type (e.g. other type is same as ours, or is a same-size + // subclass). The size check is really important to make sure we don't get + // confused about where our object boundaries are. + template ::value>> constexpr inline Span(const FixedSpan & other); constexpr pointer data() const { return mDataBuf; } @@ -169,28 +175,25 @@ class FixedSpan // // To do that we have a template constructor enabled only when the type // passed to it is a pointer type, and that pointer is to a type that - // matches T up to const-ness. Importantly, we do NOT want to allow - // subclasses of T here, because they would have a different size and our - // Span would not work right. - template < - class U, - typename = std::enable_if_t::value && - std::is_same, std::remove_const_t>>::value>> + // matches T's size and can convert to T*. + template ::value && sizeof(std::remove_pointer_t) == sizeof(T) && + std::is_convertible::value>> constexpr explicit FixedSpan(U databuf) : mDataBuf(databuf) {} - template , std::remove_const_t>::value>> + template ::value>> constexpr explicit FixedSpan(U (&databuf)[M]) : mDataBuf(databuf) { static_assert(M >= N, "Passed-in buffer too small for FixedSpan"); } - template , std::remove_const_t>::value>> + template ::value>> constexpr FixedSpan(std::array & arr) : mDataBuf(arr.data()) {} // Allow implicit construction from a FixedSpan of sufficient size over a - // type that matches our type, up to const-ness. - template , std::remove_const_t>::value>> + // type that has the same size as ours, as long as the pointers are convertible. + template ::value>> constexpr FixedSpan(FixedSpan const & other) : mDataBuf(other.data()) { static_assert(M >= N, "Passed-in FixedSpan is smaller than we are"); diff --git a/src/lib/support/tests/TestSpan.cpp b/src/lib/support/tests/TestSpan.cpp index 689226225121be..b01bafb2aaec82 100644 --- a/src/lib/support/tests/TestSpan.cpp +++ b/src/lib/support/tests/TestSpan.cpp @@ -302,14 +302,48 @@ static void TestFromCharString(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, s1.data_equal(CharSpan(str, 3))); } +static void TestConversionConstructors(nlTestSuite * inSuite, void * inContext) +{ + struct Foo + { + int member = 0; + }; + struct Bar : public Foo + { + }; + + Bar objects[2]; + + // Check that various things here compile. + Span span1(objects); + Span span2(&objects[0], 1); + FixedSpan span3(objects); + FixedSpan span4(objects); + + Span testSpan1(objects); + FixedSpan testSpan2(objects); + + Span span5(testSpan1); + Span span6(testSpan2); + + FixedSpan span7(testSpan2); +} + #define NL_TEST_DEF_FN(fn) NL_TEST_DEF("Test " #fn, fn) /** * Test Suite. It lists all the test functions. */ -static const nlTest sTests[] = { NL_TEST_DEF_FN(TestByteSpan), NL_TEST_DEF_FN(TestMutableByteSpan), - NL_TEST_DEF_FN(TestFixedByteSpan), NL_TEST_DEF_FN(TestSpanOfPointers), - NL_TEST_DEF_FN(TestSubSpan), NL_TEST_DEF_FN(TestFromZclString), - NL_TEST_DEF_FN(TestFromCharString), NL_TEST_SENTINEL() }; +static const nlTest sTests[] = { + NL_TEST_DEF_FN(TestByteSpan), + NL_TEST_DEF_FN(TestMutableByteSpan), + NL_TEST_DEF_FN(TestFixedByteSpan), + NL_TEST_DEF_FN(TestSpanOfPointers), + NL_TEST_DEF_FN(TestSubSpan), + NL_TEST_DEF_FN(TestFromZclString), + NL_TEST_DEF_FN(TestFromCharString), + NL_TEST_DEF_FN(TestConversionConstructors), + NL_TEST_SENTINEL(), +}; int TestSpan() {