-1

I have a class on a header file, which has its members defined inside a pimpl class. The idea is that I use this method (basically std::aligned_storage_t and a pointer, but the size and alignment of the class have to be specified when declaring the object) to allocate the pimpl class on the stack. I want to make the code cross-compiler so guessing isn't an option, thus I defined 2 private static constexpr functions: impl_size and impl_align which are defined on the corresponding source file and basically return sizeof(pimpl) and alignof(pimpl). The problemm is that I get the following error from MSVC (not tested on other compilers):

expression must have a constant value -- constexpr function function "Type::impl_size" (declared at line 70) is not defined

line 70 is where impl_size is defined on the header.

MCVE:

#include <cstddef>

template <typename T1, std::size_t N1, std::size_t N2>
struct Pimpl;

class Type
{
  private:
    static constexpr std::size_t impl_size() noexcept; // line 70
    static constexpr std::size_t impl_align() noexcept;

    struct impl {};

    Pimpl<impl, impl_size(), impl_align()> data; // error happens here
};

constexpr std::size_t Type::impl_size() noexcept
{
    return sizeof(Type::impl);
}

constexpr std::size_t Type::impl_align() noexcept
{
    return alignof(Type::impl);
}
Asteroids With Wings
  • 17,071
  • 2
  • 21
  • 35
Terens Tare
  • 129
  • 1
  • 14
  • Please edit your post and create a [mcve]. We can't fix code that we can't see. – alter_igel Dec 13 '20 at 20:18
  • I added a minimalistic example. If it isn't enough, I am happy to "expand" it to a more complete one. – Terens Tare Dec 13 '20 at 20:41
  • Clearly, the compiler needs to know the value of `impl_size()` at compile time, and for that it needs to see its body wherever `header.hpp` is included. – Igor Tandetnik Dec 13 '20 at 20:46
  • @TerensTare A MCVE must always be complete. No exceptions. Do it from the start so we don't all waste time on a back-and-forth having to ask for it as we do on every other question we get daily – Asteroids With Wings Dec 13 '20 at 20:48

2 Answers2

2

It's what it says: you didn't define your member function when you declared it constexpr.

You must provide the definition immediately, inline in the class definition.

Asteroids With Wings
  • 17,071
  • 2
  • 21
  • 35
1

There are a couple issues with your code that strike me as odd. The point of the "PImpl" idiom is to decouple an interface from its implementation, typically in order to allow easier compilation when the implementation changes. However, by defining your struct impl inside your interface class and by using it in a template within the same class, you are essentially forcing the implementation to be coupled with with the interface.

sizeof and alignof require a complete type, which impl does not appear to be(EDIT at the time of writing, impl was simply forward-declared), and so even after fixing the issues with Type::impl_size() and Type::impl_align(), you'll encounter this problem.

An immediate and somewhat shallow fix to your problem would be to make impl a complete type (and define it at the point of declaration), and to make impl_size() and impl_align() into inline static constexpr functions that are also defined on the spot:

class type
{
  // ...

  private:
    struct impl {
        // struct definition, so that impl is a complete type
    };

    inline static constexpr impl_size() noexcept {
        return sizeof(impl);
    }
    inline static constexpr impl_align() noexcept {
        return alignof(impl);
    }
    
    Pimpl<impl, impl_size(), impl_align()> data;
};

This still smells a bit because impl_size and impl_align are pointless boilerplate, and your PImpl template can have all the same information directly from its first parameter:

template<typename T>
class Pimpl {
    static constexpr std::size_t Size = sizeof(T);
    static constexpr std::size_t Alignment = alignof(T);
};

class type {
  private:
    struct impl {};
    
    Pimpl<impl> data;
};

This of course also requires impl to be a complete type. And this will be required anyway if struct impl is a nested cless.

It appears you're trying to do some kind of type-erasure here (or is there another good reason you need the size and alignment of impl?), but have inadvertently introduced a lot of dependencies on the implementation and types involved.

I would suggest the following: forward-declare your impl class at namespace scope, and simply use a std::unique_ptr<impl> for your implementation. Then impl can be defined in your implementation file. Note that std::unique_ptr does not require a complete type.

#include <iostream>
#include <memory>

// header.hpp
struct impl;

class type {
public:
    type();
    void doThing();
    
private:
    std::unique_ptr<impl> m_pImpl;
};

// source.cpp
struct impl {
    void doThingImpl() {
        std::cout << "Did a thing";
    }
};

type::type()
    : m_pImpl(std::make_unique<impl>()) {
      
}

void type::doThing() {
    m_pImpl->doThingImpl();
}

int main(){
    auto t = type{};
    t.doThing();
    
    return 0;
}

Live Demo

alter_igel
  • 6,899
  • 3
  • 21
  • 40
  • Thanks for your answer. I am trying to avoid heap allocations (It's a "fun" project thing), but I guess I will use smart pointers. – Terens Tare Dec 13 '20 at 21:00
  • You _could_ placement-new the `impl` object inside of a block of non-heap-allocated storage in the `type` class directly, but you'd be required to hard-code the size and alignment (or upper bounds thereof) of the `impl` object for that to work (because you can't get the size or alignment of an incomplete type). It could be fun if extreme performance is your thing, but I wouldn't encourage it for memory safety and maintainability reasons. – alter_igel Dec 13 '20 at 21:03
  • That was the idea used with `std::aligned_storage_t`, but I guess I better use sth else than `std::aligned_storage_t` then. Thanks again – Terens Tare Dec 13 '20 at 21:09
  • Realistically, if heap allocation was my biggest concern, I would just implement all of `type` using ordinary member functions, even if it means slightly longer compilation here or there. – alter_igel Dec 13 '20 at 21:12
  • I'm not concerned about the compilation times, I just want to avoid exposing the members of `Type` to the header. That's because I want to build the _source.cpp_ as a DLL and I get warnings about templates instantiation from MSVC (one of the members is a `std::map`), so I thought maybe it could be better if I just "hided" these members. – Terens Tare Dec 13 '20 at 21:16
  • 1
    @TerensTare I got curious and I think I have a working demo of relatively safe placement-new PImpl: https://coliru.stacked-crooked.com/a/9237399182be0d1f The size and alignment are checked at compile time. If the checks fail, you can query them by simply defining a macro `GIVE_ME_THE_SIZE` and use the output to correct the `aligned_storage_t` – alter_igel Dec 13 '20 at 23:13
  • Thanks again @alter igel. The problem is that I want it to be cross-compiler, but since the standard doesn't put any constraint for the size of `std::` types, I can't just harcode-them. I guess compiler-specific macros will work. – Terens Tare Dec 14 '20 at 08:44
  • 1
    Upper bounds should work too, for example `impl_size` => `max_impl_size` and `static_assert(sizeof(impl) <= max_impl_size);` – alter_igel Dec 14 '20 at 17:40