11
\$\begingroup\$

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< typename T, std::size_t Align = 16>
class aligned_wrapper{
public:
    typedef T value_type;

    template<typename... Args>
    aligned_wrapper(Args&&... args){
        std::size_t space = sizeof(m_storage);
        auto raw_ptr = reinterpret_cast<void*>(&m_storage);
        auto ptr = std::align(Align, sizeof(T), raw_ptr, space);
        new (ptr)T(std::forward<Args>(args)...);
        m_object = reinterpret_cast<T*>(ptr);
    }

    ~aligned_wrapper(){
        get().~T();
    }

    aligned_wrapper& operator = (const aligned_wrapper& that){
        get() = that.get();
        return *this;
    }

    template<typename U>
    aligned_wrapper(const aligned_wrapper<U>& that)
        : aligned_wrapper(that.get())
    {}

    template<typename U>
    aligned_wrapper(aligned_wrapper<U>&& 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<sizeof(T) + Align - 1, 1> m_storage;
    T* m_object{ nullptr };
};

Although it may look a bit funky, one of the key ideas is that 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.

\$\endgroup\$
3
\$\begingroup\$
  1. 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.

  2. 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.

  3. 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.

  4. 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<T>::value) {
        get() = that.get();
        return *this;
    }
    
  5. 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;
    
  6. 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.

  7. 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>(args)...);
    
  8. 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*).

  9. 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.

  10. 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.

  11. 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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.