4
\$\begingroup\$

If you can't afford the extra level of indirection, that comes with PIMPL, use the implstore! Improvements?

#ifndef IMPLSTORE_HPP
# define IMPLSTORE_HPP
# pragma once

#include <cstddef>

#include <new>

#include <type_traits>

#include <utility>

template <class U, ::std::size_t N = 64>
class implstore
{
public:
  implstore()
  {
    static_assert(sizeof(U) <= sizeof(store_),
      "impl too large");
    static_assert(::std::is_default_constructible<U>{},
      "impl is not default constructible");
    new (store_) U;

    deleter_ = deleter_stub;
  }

  template <typename ...A>
  explicit implstore(A&& ...args)
  {
    static_assert(sizeof(U) <= sizeof(store_),
      "impl too large");
    new (store_) U(::std::forward<A>(args)...);

    deleter_ = deleter_stub;
  }

  ~implstore() { *this && (deleter_(*this), true); }

  template <::std::size_t M>
  implstore(implstore<U, M> const& other)
  {
    static_assert(::std::is_copy_constructible<U>{},
      "impl is not copy constructible");
    new (store_) U(*other);

    deleter_ = other.deleter_;
  }

  template <::std::size_t M>
  implstore(implstore<U, M>&& other)
  {
    static_assert(::std::is_move_constructible<U>{},
      "impl is not move constructible");

    new (store_) U(*other);

    deleter_ = other.deleter_;
  }

  template <::std::size_t M>
  implstore& operator=(implstore<U, M> const& other)
  {
    static_assert(::std::is_copy_assignable<U>{},
      "impl is not copy assignable");

    **this = *other;

    deleter_ = other.deleter_;

    return *this;
  }

  template <::std::size_t M>
  implstore& operator=(implstore<U, M>&& other)
  {
    static_assert(::std::is_move_assignable<U>{},
      "impl is not move assignable");

    **this = ::std::move(*other);

    deleter_ = other.deleter_;

    return *this;
  }

  U const* operator->() const noexcept
  {
    return reinterpret_cast<U*>(store_);
  }

  U* operator->() noexcept
  {
    return reinterpret_cast<U*>(store_);
  }

  U const* get() const noexcept
  {
    return reinterpret_cast<U*>(store_);
  }

  U* get() noexcept
  {
    return reinterpret_cast<U*>(store_);
  }

  U const& operator*() const noexcept
  {
    return *reinterpret_cast<U*>(store_);
  }

  U& operator*() noexcept
  {
    return *reinterpret_cast<U*>(store_);
  }

  explicit operator bool() const noexcept { return deleter_; }

private:
  static void deleter_stub(implstore& is)
  {
    typedef char type_must_be_complete[sizeof(U) ? 1 : -1];
    (void)sizeof(type_must_be_complete);
    is->~U();
  }

private:
  void (*deleter_)(implstore&){};

  alignas(::std::max_align_t) char store_[N];
};

#endif // IMPLSTORE_HPP

Edit. Here is the version built around Dyps' suggestions. From what I understood, Dyp suggests the implstore class to inherit the type traits of the Impl class it wraps, as much as possible. In order for this thing to work you need to declare and define (in the containing class) your own destructor, whatever copy constructors you might need and whatever assignment operators you might need.

#ifndef IMPLSTORE_HPP
# define IMPLSTORE_HPP
# pragma once

#include <cstddef>

#include <new>

#include <type_traits>

#include <utility>

template <class U, ::std::size_t N = 64>
class implstore
{
public:
  static constexpr ::std::size_t const buffer_size = N;

  using value_type = U;

  template <typename ...A, typename =
    typename ::std::enable_if<::std::is_constructible<U, A...>{}>::type>
  implstore(A&& ...args)
  {
    static_assert(sizeof(U) <= sizeof(store_),
      "impl too large");
    new (static_cast<void*>(&store_)) U(::std::forward<A>(args)...);
  }

  ~implstore() { get()->~U(); }

  implstore(implstore const& other)
  {
    new (static_cast<void*>(&store_)) U(*other);
  }

  template <::std::size_t M, typename K = U, typename =
    typename ::std::enable_if<::std::is_copy_constructible<K>{}>::type>
  implstore(implstore<U, M> const& other)
  {
    new (static_cast<void*>(&store_)) U(*other);
  }

  template <::std::size_t M, typename K = U, typename =
    typename ::std::enable_if<::std::is_move_constructible<K>{}>::type>
  implstore(implstore<U, M>&& other)
  {
    new (static_cast<void*>(&store_)) U(::std::move(*other));
  }

  implstore& operator=(implstore const& other)
  {
    **this = *other;

    return *this;
  }

  template <::std::size_t M, typename K = U, typename =
    typename ::std::enable_if<::std::is_copy_assignable<K>{}>::type>
  implstore& operator=(implstore<U, M> const& other)
  {
    **this = *other;

    return *this;
  }

  template <::std::size_t M, typename K = U, typename =
    typename ::std::enable_if<::std::is_move_assignable<K>{}>::type>
  implstore& operator=(implstore<U, M>&& other)
  {
    **this = ::std::move(*other);

    return *this;
  }

  U const* operator->() const noexcept
  {
    return reinterpret_cast<U const*>(&store_);
  }

  U* operator->() noexcept
  {
    return reinterpret_cast<U*>(&store_);
  }

  U const* get() const noexcept
  {
    return reinterpret_cast<U const*>(&store_);
  }

  U* get() noexcept
  {
    return reinterpret_cast<U*>(&store_);
  }

  U const& operator*() const noexcept
  {
    return *reinterpret_cast<U const*>(&store_);
  }

  U& operator*() noexcept
  {
    return *reinterpret_cast<U*>(&store_);
  }

private:
  typename ::std::aligned_storage<N>::type store_;
};

#endif // IMPLSTORE_HPP
\$\endgroup\$
22
  • \$\begingroup\$ Maybe it would help if you explained what you're trying to do... \$\endgroup\$ May 16 '14 at 16:12
  • 1
    \$\begingroup\$ I think you could reduce the alignment requirements to the max requirements for a type of the max size. This is what std::aligned_storage does per default. \$\endgroup\$
    – dyp
    May 16 '14 at 19:15
  • \$\begingroup\$ Why did you use this: *this && (deleter_(*this), true) Is there any particular reason not to write if(*this) deleter_(*this)? \$\endgroup\$
    – dyp
    May 16 '14 at 19:31
  • 1
    \$\begingroup\$ Are you using the deleter_ to make sure U is complete when calling its dtor? \$\endgroup\$
    – dyp
    May 16 '14 at 19:36
  • \$\begingroup\$ @dyp I think new cannot operate on incomplete types, as for deleter_, no, I was merely trying to make sure, the destructor of object contained in the store_ was called. As for alignment requirements, please write an answer/fix, I'll gladly accept. \$\endgroup\$ May 16 '14 at 21:47
4
\$\begingroup\$
new (store_) U;

You should explicitly cast to a void* to avoid calling any user-defined allocation functions instead of placement-new.

void (*deleter_)(implstore&){};

To my eyes, this is quite ugly. It almost looks like a member function, but it's a pointer. I'd prefer

using deleter_t = void(*)(implstore&);
deleter_t deleter_ = nullptr;

But I think both the NSDMI (= nullptr or {}) and deleter_ itself is unnecessary. Since you now changed the code so that every constructor initializes the stored object, you don't even need it as a check whether or not the object is initialized. If you leave out the deleter_, you force the user of implstore to define their dtor out-of-line at a point where the type stored inside the implstore is complete. If the user doesn't do that, the error message is not very nice (destructor of incomplete type called at the point where the class using implstore is destroyed), but it saves you 8 bytes per object. The cost of the function call has to be paid anyway AFAIK.

template <typename ...A>
explicit implstore(A&& ...args)

Typically, such constructors are too greedy and should be restricted by SFINAE via std::is_constructible so that they only produce instantiations for arguments for which the stored object can be constructed. Otherwise, implstore will appear constructible from any argument set (std::is_constructible<implstore, ..> will always yield true).

Additionally, constructor templates and assignment-operator templates do not suppress the generation of compiler-generated copy/move constructors and assignment-operators.


~implstore() { *this && (deleter_(*this), true); }

I'd prefer

~implstore() { if(*this) deleter_(*this); }

which is shorter and (arguably) clearer. But neither is necessary with your changed code:

~implstore() { get()->~U(); }

should be sufficient.

Here's an example of how it can/needs to be used:

// MyClass.hpp
class MyClass
{
private:
    struct Impl;
    implstore<Impl, 10> impl;
public:
    MyClass();
    MyClass(MyClass const&);
    ~MyClass();
    MyClass& operator=(MyClass const&);

    void meow();
};


// MyClass.cpp
#include <iostream>
#include "MyClass.hpp"

struct MyClass::Impl
{
    void meow() { std::cout << "meow!\n"; }
};

MyClass::MyClass() = default;
MyClass::MyClass(MyClass const&) = default;
MyClass::~MyClass() = default;
MyClass& MyClass::operator=(MyClass const&) = default;

void MyClass::meow() { impl.get()->meow(); }


// other.cpp
#include "MyClass.hpp"

int main()
{
    MyClass m;
    m.meow();
}

The important part of course is the out-of-line definition of all member functions of MyClass that require Impl to be complete. This includes the destructor.


template <::std::size_t M>
implstore(implstore<U, M> const& other)

.. and other constructors: I'd use constructor delegation or an initialization function to get rid of the duplicated code. Similarly,

U const* operator->() const noexcept
{
  return reinterpret_cast<U*>(store_);
}

and all other variations can use a single (private const) member function instead of repeating that cast.

alignas(::std::max_align_t) char store_[N];

Can, I think, be reduced to

typename std::aligned_storage<N>::type store_;

But I'm not sure about the aliasing correctness in either case (especially combined with the reinterpret_casts). But that's probably a StackOverflow question.


Furthermore, I miss the publication of the template arguments. As a user, you can always get them out of the type by partial specialization, so they're not private anyway.

using value_type = U;
static constexpr ::std::size_t buffer_size = N;

And you could use those names instead of template parameters inside the class itself (arguably "nicer" and decouples the types from the template parameters).

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Not sure what to think about ::std. Not sure how to search for previous discussion on this topic, either :/ \$\endgroup\$
    – dyp
    May 16 '14 at 23:01
  • \$\begingroup\$ If this worked ~implstore() { get()->~U(); } it would be great, but compiler complains about an incomplete type. I need to support incomplete types, so the deleter_ indirection appears to be necessary for a successful compile. \$\endgroup\$ May 17 '14 at 10:22
  • \$\begingroup\$ @user1095108 I've added an example that compiles with clang++3.3 \$\endgroup\$
    – dyp
    May 17 '14 at 11:56

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.