Skip to content

Commit

Permalink
COMMON: Simplify Span code
Browse files Browse the repository at this point in the history 
Implicitly generated constructors can be used instead of explicit
constructors, which reduces the amount of necessary boilerplate.

Long lists of identical typedefs to the superclass are now defined
using a macro.

data() const now returns a pointer to data that matches the
value_type of the data, instead of forcing the data to be const.
This better matches the intent of the Span class, which provides
a view into data, rather than being a container that holds data.
  • Loading branch information
@csnover
csnover committed Jan 8, 2017
1 parent 9c60bcf commit 3cfc396
Show file tree
Hide file tree
Showing 2 changed files with 117 additions and 110 deletions.
149 changes: 41 additions & 108 deletions common/span.h
View file
Expand Up @@ -31,6 +31,18 @@

namespace Common {

#define COMMON_SPAN_TYPEDEFS \
typedef typename super_type::value_type value_type; \
typedef typename super_type::difference_type difference_type; \
typedef typename super_type::index_type index_type; \
typedef typename super_type::size_type size_type; \
typedef typename super_type::const_iterator const_iterator; \
typedef typename super_type::iterator iterator; \
typedef typename super_type::pointer pointer; \
typedef typename super_type::const_pointer const_pointer; \
typedef typename super_type::reference reference; \
typedef typename super_type::const_reference const_reference;

enum {
kSpanMaxSize = 0xFFFFFFFF,
kSpanKeepOffset = 0xFFFFFFFF
Expand Down Expand Up @@ -271,10 +283,10 @@ class SpanBase : public SafeBool<Derived<ValueType> > {
#if !defined(__GNUC__) || GCC_ATLEAST(3, 0)
protected:
#endif
SpanBase() {}
SpanBase(const SpanBase &) {}
void operator=(const SpanBase &) {}
~SpanBase() {}
inline SpanBase() {}
inline SpanBase(const SpanBase &) {}
inline SpanBase &operator=(const SpanBase &) { return this->impl(); }
inline ~SpanBase() {}

inline const_derived_type &impl() const { return static_cast<const_derived_type &>(*this); }
inline mutable_derived_type &impl() { return static_cast<mutable_derived_type &>(*this); }
Expand All @@ -296,8 +308,7 @@ class SpanBase : public SafeBool<Derived<ValueType> > {
inline iterator begin();
inline iterator end();

inline const_pointer data() const;
inline pointer data();
inline pointer data() const;

#pragma mark -
#pragma mark SpanBase - Data access functions
Expand Down Expand Up @@ -498,6 +509,9 @@ class SpanBase : public SafeBool<Derived<ValueType> > {
#if !defined(__GNUC__) || GCC_ATLEAST(3, 0)
protected:
#endif
/**
* @returns true if bounds are invalid.
*/
inline bool checkInvalidBounds(const index_type index, const difference_type deltaInBytes) const {
// There is a potential that large bogus values may cause arithmetic
// overflow, so the individual operands are checked separately first.
Expand All @@ -508,9 +522,11 @@ class SpanBase : public SafeBool<Derived<ValueType> > {
}

inline void validate(const index_type index, const difference_type deltaInBytes, const SpanValidationMode mode = kValidateRead) const {
/* LCOV_EXCL_START */
if (impl().checkInvalidBounds(index, deltaInBytes)) {
error("%s", impl().getValidationMessage(index, deltaInBytes, mode).c_str()); /* LCOV_EXCL_LINE */
error("%s", impl().getValidationMessage(index, deltaInBytes, mode).c_str());
}
/* LCOV_EXCL_STOP */
}
};

Expand All @@ -531,16 +547,7 @@ class SpanImpl : public SpanBase<ValueType, Derived> {
#endif

public:
typedef typename super_type::value_type value_type;
typedef typename super_type::difference_type difference_type;
typedef typename super_type::index_type index_type;
typedef typename super_type::size_type size_type;
typedef typename super_type::const_iterator const_iterator;
typedef typename super_type::iterator iterator;
typedef typename super_type::pointer pointer;
typedef typename super_type::const_pointer const_pointer;
typedef typename super_type::reference reference;
typedef typename super_type::const_reference const_reference;
COMMON_SPAN_TYPEDEFS

inline SpanImpl() : super_type(), _data(nullptr), _size(0) {}

Expand All @@ -555,25 +562,13 @@ class SpanImpl : public SpanBase<ValueType, Derived> {
_data(other.data()),
_size(other.size()) {}

template <typename Other>
inline mutable_derived_type &operator=(const Other &other) {
// TODO: Is there a better way to do this which avoids casting away
// const in the case that value_type is explicitly defined const?
_data = const_cast<typename Other::pointer>(other.data());
_size = other.size();
return this->impl();
}

inline ~SpanImpl() {}

inline void clear() {
_data = nullptr;
_size = 0;
}

inline size_type size() const { return _size; }
inline const_pointer data() const { return _data; }
inline pointer data() { return _data; }
inline pointer data() const { return _data; }

inline const_iterator cbegin() const { return const_iterator(&this->impl(), 0); }
inline const_iterator cend() const { return const_iterator(&this->impl(), size()); }
Expand Down Expand Up @@ -679,9 +674,9 @@ class SpanImpl : public SpanBase<ValueType, Derived> {
numEntries = (stream.size() - stream.pos()) / sizeof(value_type);
}

assert(stream.pos() + numEntries * sizeof(value_type) <= (uint)stream.size());
allocate(numEntries);
const uint32 bytesRequested = numEntries * sizeof(value_type);
assert(stream.pos() + bytesRequested <= (uint)stream.size());
allocate(numEntries);
const uint32 bytesRead = stream.read((void *)const_cast<mutable_value_type *>(_data), bytesRequested);
assert(bytesRead == bytesRequested);
return (mutable_value_derived_type &)const_cast<Derived<value_type> &>(this->impl());
Expand All @@ -704,31 +699,16 @@ class Span : public SpanImpl<ValueType, Span> {
#endif

public:
typedef typename super_type::value_type value_type;
typedef typename super_type::difference_type difference_type;
typedef typename super_type::index_type index_type;
typedef typename super_type::size_type size_type;
typedef typename super_type::const_iterator const_iterator;
typedef typename super_type::iterator iterator;
typedef typename super_type::pointer pointer;
typedef typename super_type::const_pointer const_pointer;
typedef typename super_type::reference reference;
typedef typename super_type::const_reference const_reference;
COMMON_SPAN_TYPEDEFS

inline Span() : super_type() {}

inline Span(const pointer data_, const size_type size_) : super_type(data_, size_) {}

// Allows unrelated sibling classes like NamedSpan to assign to superclass
// siblings like Span
template <typename Other>
inline Span(const Other &other) : super_type(other) {}

template <typename Other>
inline mutable_derived_type &operator=(const Other &other) {
super_type::operator=(other);
return this->impl();
}

inline ~Span() {}
};

#pragma mark -
Expand All @@ -748,16 +728,7 @@ class NamedSpanImpl : public SpanImpl<ValueType, Derived> {
#endif

public:
typedef typename super_type::value_type value_type;
typedef typename super_type::difference_type difference_type;
typedef typename super_type::index_type index_type;
typedef typename super_type::size_type size_type;
typedef typename super_type::const_iterator const_iterator;
typedef typename super_type::iterator iterator;
typedef typename super_type::pointer pointer;
typedef typename super_type::const_pointer const_pointer;
typedef typename super_type::reference reference;
typedef typename super_type::const_reference const_reference;
COMMON_SPAN_TYPEDEFS

inline NamedSpanImpl() : super_type(), _name(), _sourceByteOffset(0) {}

Expand All @@ -769,36 +740,12 @@ class NamedSpanImpl : public SpanImpl<ValueType, Derived> {
_name(name),
_sourceByteOffset(sourceByteOffset) {}

template <typename OtherValueType>
inline NamedSpanImpl(const NamedSpanImpl<OtherValueType, Derived> &other) :
template <typename Other>
inline NamedSpanImpl(const Other &other) :
super_type(other),
_name(other.name()),
_sourceByteOffset(other.sourceByteOffset()) {}

template <typename OtherValueType>
inline NamedSpanImpl(const SpanImpl<OtherValueType, Derived> &other) :
super_type(other),
_name(String::format("%p", const_cast<const void *>(other.data()))),
_sourceByteOffset(0) {}

template <typename OtherValueType>
inline mutable_derived_type &operator=(const NamedSpanImpl<OtherValueType, Derived> &other) {
super_type::operator=(other);
_name = other.name();
_sourceByteOffset = other.sourceByteOffset();
return this->impl();
}

template <typename OtherValueType>
inline mutable_derived_type &operator=(const SpanImpl<OtherValueType, Derived> &other) {
super_type::operator=(other);
_name = String::format("%p", const_cast<const void *>(other.data()));
_sourceByteOffset = 0;
return this->impl();
}

inline ~NamedSpanImpl() {}

inline void clear() {
super_type::clear();
_name.clear();
Expand Down Expand Up @@ -923,24 +870,13 @@ class NamedSpanImpl : public SpanImpl<ValueType, Derived> {
template <typename ValueType>
class NamedSpan : public NamedSpanImpl<ValueType, NamedSpan> {
typedef NamedSpanImpl<ValueType, ::Common::NamedSpan> super_type;
typedef typename AddConst<NamedSpan<ValueType> >::type const_derived_type;
typedef typename RemoveConst<NamedSpan<ValueType> >::type mutable_derived_type;

#if !defined(__GNUC__) || GCC_ATLEAST(3, 0)
template <typename T> friend class NamedSpan;
#endif

public:
typedef typename super_type::value_type value_type;
typedef typename super_type::difference_type difference_type;
typedef typename super_type::index_type index_type;
typedef typename super_type::size_type size_type;
typedef typename super_type::const_iterator const_iterator;
typedef typename super_type::iterator iterator;
typedef typename super_type::pointer pointer;
typedef typename super_type::const_pointer const_pointer;
typedef typename super_type::reference reference;
typedef typename super_type::const_reference const_reference;
COMMON_SPAN_TYPEDEFS

inline NamedSpan() : super_type() {}

Expand All @@ -952,14 +888,6 @@ class NamedSpan : public NamedSpanImpl<ValueType, NamedSpan> {

template <typename Other>
inline NamedSpan(const Other &other) : super_type(other) {}

template <typename Other>
inline mutable_derived_type &operator=(const Other &other) {
super_type::operator=(other);
return this->impl();
}

inline ~NamedSpan() {}
};

#pragma mark -
Expand All @@ -974,6 +902,7 @@ class SpanOwner : public SafeBool<SpanOwner<OwnedSpan> > {
typedef typename OwnedSpan::value_type value_type;
typedef typename OwnedSpan::size_type size_type;
typedef typename OwnedSpan::index_type index_type;
typedef typename OwnedSpan::pointer pointer;
typedef typename OwnedSpan::reference reference;
typedef typename OwnedSpan::const_reference const_reference;

Expand Down Expand Up @@ -1006,6 +935,10 @@ class SpanOwner : public SafeBool<SpanOwner<OwnedSpan> > {
* If this owner already holds another Span, the old Span will be destroyed.
*/
inline SpanOwner &operator=(SpanOwner &other) {
if (this == &other) {
return *this;
}

if (_span.data()) {
delete[] const_cast<typename RemoveConst<value_type>::type *>(_span.data());
}
Expand All @@ -1021,8 +954,8 @@ class SpanOwner : public SafeBool<SpanOwner<OwnedSpan> > {
/**
* Releases the memory owned by this SpanOwner to the caller.
*/
inline value_type *release() {
value_type *data = _span.data();
inline pointer release() {
pointer data = _span.data();
_span.clear();
return data;
}
Expand Down
78 changes: 76 additions & 2 deletions test/common/span.h
View file
Expand Up @@ -10,7 +10,75 @@ class SpanTestSuite : public CxxTest::TestSuite {
int a;
};

template <typename ValueType, template <typename> class Derived>
class SiblingSpanImpl : public Common::SpanImpl<ValueType, Derived> {
typedef Common::SpanImpl<ValueType, Derived> super_type;
public:
COMMON_SPAN_TYPEDEFS
SiblingSpanImpl() : super_type() {}
SiblingSpanImpl(pointer data_, size_type size_) : super_type(data_, size_) {}
};

template <typename ValueType>
class SiblingSpan : public SiblingSpanImpl<ValueType, SiblingSpan> {
typedef SiblingSpanImpl<ValueType, ::SpanTestSuite::SiblingSpan> super_type;
public:
COMMON_SPAN_TYPEDEFS
SiblingSpan() : super_type() {}
SiblingSpan(pointer data_, size_type size_) : super_type(data_, size_) {}
};

template <typename ValueType, template <typename> class Derived>
class SubSpanImpl : public Common::NamedSpanImpl<ValueType, Derived> {
typedef Common::NamedSpanImpl<ValueType, Derived> super_type;
public:
COMMON_SPAN_TYPEDEFS
SubSpanImpl() : super_type() {}
SubSpanImpl(pointer data_,
size_type size_,
const Common::String &name_ = Common::String(),
const size_type sourceByteOffset_ = 0) :
super_type(data_, size_, name_, sourceByteOffset_) {}

template <typename Other>
SubSpanImpl(const Other &other) : super_type(other) {}
};

template <typename ValueType>
class SubSpan : public SubSpanImpl<ValueType, SubSpan> {
typedef SubSpanImpl<ValueType, ::SpanTestSuite::SubSpan> super_type;
public:
COMMON_SPAN_TYPEDEFS
SubSpan() : super_type() {}
SubSpan(pointer data_,
size_type size_,
const Common::String &name_ = Common::String(),
const size_type sourceByteOffset_ = 0) :
super_type(data_, size_, name_, sourceByteOffset_) {}

template <typename Other>
SubSpan(const Other &other) : super_type(other) {}
};

public:
void test_sibling_span() {
byte data[] = { 'h', 'e', 'l', 'l', 'o' };
SiblingSpan<byte> ss(data, sizeof(data));
Common::Span<byte> superInstance = ss;
TS_ASSERT_EQUALS(ss.data(), data);
TS_ASSERT_EQUALS(superInstance.data(), data);
}

void test_sub_span() {
byte data[] = { 'h', 'e', 'l', 'l', 'o' };
SubSpan<byte> ss(data, sizeof(data), "custom subspan");
Common::NamedSpan<byte> namedSuper = ss;
Common::Span<byte> unnamedSuper = ss;
TS_ASSERT(ss.name() == "custom subspan");
TS_ASSERT(namedSuper.name() == ss.name());
TS_ASSERT(unnamedSuper.name() == Common::String::format("%p", (void *)data));
}

void test_span_iterator_const() {
byte data[] = { 'h', 'e', 'l', 'l', 'o' };
const Common::Span<byte> span(data, sizeof(data));
Expand Down Expand Up @@ -210,6 +278,11 @@ class SpanTestSuite : public CxxTest::TestSuite {

// tests destruction of held pointer by reassignment
owner2 = owner;

// tests nullipotence of assignment to self
dataPtr = owner2->data();
owner2 = owner2;
TS_ASSERT(owner2->data() == dataPtr);
}

{
Expand Down Expand Up @@ -561,8 +634,9 @@ class SpanTestSuite : public CxxTest::TestSuite {
TS_ASSERT(!span.checkInvalidBounds(6, 0));
TS_ASSERT(!span.checkInvalidBounds(2, -2));
TS_ASSERT(span.checkInvalidBounds(-2, 2)); // negative index disallowed
TS_ASSERT(span.checkInvalidBounds(6, 1)); // positive overflow (+7)
TS_ASSERT(span.checkInvalidBounds(6, 1)); // combined positive overflow (+7)
TS_ASSERT(span.checkInvalidBounds(2, -4)); // negative overflow (-2)
TS_ASSERT(span.checkInvalidBounds(0, 10)); // delta positive overflow

const ptrdiff_t big = 1L << (8 * sizeof(ptrdiff_t) - 1);
TS_ASSERT(span.checkInvalidBounds(big, 0));
Expand Down Expand Up @@ -653,7 +727,7 @@ class SpanTestSuite : public CxxTest::TestSuite {
}

Common::NamedSpan<byte> span2;
span2 = span;
span = span2 = span;
TS_ASSERT_EQUALS(span2, span);
TS_ASSERT(span2.name() == span.name());
TS_ASSERT(span2.sourceByteOffset() == span.sourceByteOffset());
Expand Down

0 comments on commit 3cfc396

Please sign in to comment.