HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Wrapping types with alignment requirements

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withalignmentwrappingtypesrequirements

Problem

Visual Studio 2013 still doesn't support the 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 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.