patterncppMinor
Wrapping types with alignment requirements
Viewed 0 times
withalignmentwrappingtypesrequirements
Problem
Visual Studio 2013 still doesn't support the
To solve these errors and warnings I'm implementing a wrapper type that will wrap another type with some alignment restriction regardless of how the wrapper is aligned. Consider two wrapper objects
I'm looking for a review on the code on the account of syntax and possible weaknesses in the design and implementation.
Although it may look a bit funky, one
alignas keyword in C++11. This causes some problems with alignment of types in various situations. Thankfully the compiler will generate an error when it can't guarantee the alignment or at least give a warning. To solve these errors and warnings I'm implementing a wrapper type that will wrap another type with some alignment restriction regardless of how the wrapper is aligned. Consider two wrapper objects
A and B that themselves have arbitrary alignment. Let the wrapped datum be a and b respectively. Then A=B will assign a=b while still keeping a aligned regardless of how B was aligned because b in itself is aligned.I'm looking for a review on the code on the account of syntax and possible weaknesses in the design and implementation.
template
class aligned_wrapper{
public:
typedef T value_type;
template
aligned_wrapper(Args&&... args){
std::size_t space = sizeof(m_storage);
auto raw_ptr = reinterpret_cast(&m_storage);
auto ptr = std::align(Align, sizeof(T), raw_ptr, space);
new (ptr)T(std::forward(args)...);
m_object = reinterpret_cast(ptr);
}
~aligned_wrapper(){
get().~T();
}
aligned_wrapper& operator = (const aligned_wrapper& that){
get() = that.get();
return *this;
}
template
aligned_wrapper(const aligned_wrapper& that)
: aligned_wrapper(that.get())
{}
template
aligned_wrapper(aligned_wrapper&& that)
: aligned_wrapper(std::move(that.get()))
{}
inline T& get(){ return *m_object; }
inline const T& get() const { return *m_object; }
void swap(aligned_wrapper& that){
using std::swap;
swap(get(), that.get());
}
friend void swap(aligned_wrapper& lhs, aligned_wrapper& rhs){
lhs.swap(rhs);
}
private:
std::aligned_storage_t m_storage;
T* m_object{ nullptr };
};Although it may look a bit funky, one
Solution
-
The class is missing a copy constructor and a move constructor. These can never be created by instantiating a template. Right now, the compiler will generate a defaulted copy constructor for you (and no move constructor, because you have a user-defined destructor). Since you obviously don't want that, you have to define the copy (and probably move as well) constructor yourself.
-
You're also missing a move assignment operator which you should probably add. Otherwise, your class wouldn't be fully usable with move-only types.
-
Your code is missing comments (there aren't any). You even had to explain an invariant in the question:
... once the
This type of information must go into comments. The next maintainer (which can be you in 6 moths' time) will need knowledge like this to understand the code.
-
You should add
-
It would probably be a good idea to introduce a way to query
-
You don't need
-
You can simplify the assignment to
-
-
There is no reason to use
As a side note, be aware that
-
You might want to swap the order of the data members, it might prevent need for padding between an oddly-sized
-
You might consider overloading the pointer operators
The class is missing a copy constructor and a move constructor. These can never be created by instantiating a template. Right now, the compiler will generate a defaulted copy constructor for you (and no move constructor, because you have a user-defined destructor). Since you obviously don't want that, you have to define the copy (and probably move as well) constructor yourself.
-
You're also missing a move assignment operator which you should probably add. Otherwise, your class wouldn't be fully usable with move-only types.
-
Your code is missing comments (there aren't any). You even had to explain an invariant in the question:
... once the
aligned_wrapper is constructed, m_object will never change for that instance as the address of this will not change, this guarantees alignment. In other words, not assigning m_object in the assignment operator is intentional.This type of information must go into comments. The next maintainer (which can be you in 6 moths' time) will need knowledge like this to understand the code.
-
You should add
noexcept specifications to your functions. They affect efficiency of some operations (like storing in a std::vector), and since this is a very generic class, you should not limit its use in any way. An example for the assignment operator would be:aligned_wrapper& operator = (const aligned_wrapper& that) noexcept(std::is_nothrow_copy_assignable::value) {
get() = that.get();
return *this;
}-
It would probably be a good idea to introduce a way to query
Align out of an aligned_wrapper object, e.g.static const std::size_t alignment = Align;-
You don't need
reinterpret_cast to convert an object pointer to void*, a static_cast is sufficient for that. Don't use stronger casts than necessary.-
You can simplify the assignment to
m_object and make it more obvious by assigning the result of the placement-new into it:m_object = new (ptr) T(std::forward(args)...);-
16 looks like a rather arbitrary number for the default template argument for Align (it's actually a magic number). There is even no guarantee that 16 is a valid alignment value. I would probably choose something less arbitrary such as sizeof(void*).-
There is no reason to use
std::aligned_storage for the type of m_storage. You don't actually care about alignment of m_storage at all (which is emphasised by you passing 1 to its Align parameter). All you need is a buffer of suitable size, and for this usigned char[sizeof(T) + Align -1] (or a std::array) would work just as well.As a side note, be aware that
std::aligned_storage_t was only introduced in C++14. Visual Studio 2013 supports some C++14 bits, but using it makes your code non-C++11-compliant.-
You might want to swap the order of the data members, it might prevent need for padding between an oddly-sized
m_storage and aligned m_object.-
You might consider overloading the pointer operators
* and -> for your class as syntactic sugar for get(), it simplifies usage. Other generic classes which store a single object sometimes do so as well; an example is boost::optional.Code Snippets
aligned_wrapper& operator = (const aligned_wrapper& that) noexcept(std::is_nothrow_copy_assignable<T>::value) {
get() = that.get();
return *this;
}static const std::size_t alignment = Align;m_object = new (ptr) T(std::forward<Args>(args)...);Context
StackExchange Code Review Q#67835, answer score: 3
Revisions (0)
No revisions yet.