std::unique_ptr pimpl in dll generates C4251 with visual studio
Asked Answered
Z

3

23

This is not a breaking issue but I like to clean my code from warnings so this is getting on my nerves.

I have been using the c++11 version of pimpl idiom to hide the class implementation for my library the usual way.

// dll header
class FrameworkImpl;

class EXPORT_API Framework
{
    Framework(const Framework&) = delete;
    Framework& operator=(const Framework&) = delete;
    Framework(Framework&&) = delete;
    Framework& operator=(Framework&&) = delete;

public:
    Framework();
    ~Framework();

private:
    std::unique_ptr<FrameworkImpl> impl_;
};

// application implementation
int main()
{
    std::unique_ptr<Framework> test = std::make_unique<Framework>();
}

Everything will be fine, but I will keep getting the warning:

warning C4251: 'Framework::impl_': class 'std::unique_ptr<FrameworkImpl,std::default_delete<_Ty>>' needs to have dll-interface to be used by clients of class 'Framework'

So I tried have to add:

template class EXPORT_API std::unique_ptr<FrameworkImpl>;

Before the forward declaration but the warning would just change to:

warning C4251: 'std::_Unique_ptr_base<_Ty,_Dx>::_Mypair': class 'std::_Compressed_pair<_Dx,FrameworkImpl *,true>' needs to have dll-interface to be used by clients of class 'std::_Unique_ptr_base<_Ty,_Dx>'

I have being seeing this issue since VS2010 and I cannot figure a good way to fix this. No problems on gcc or clang and it would break my heart to use the old raw pointer version..

Zimmer answered 19/8, 2015 at 14:14 Comment(3)
This answer may help: #24635755Metagnathous
As a test, can you replace std::unique_ptr<FrameworkImpl> impl_; with struct do_nothing{ template<class T> void operator()(T&&)const{}; }; std::unique_ptr<FrameworkImpl, do_nothing> impl_; -- I want to see if it is related to the default deleter. (if this does compile, you aren't done! but it is diagnostic.) Also note that the warning might be spurious.Yt
@Yakk Same warning but with 'std::unique_ptr<FrameworkImpl,Framework::do_nothing>' insteadZimmer
Z
0

The solution was to declare the constructor/destructor after the impl declaration and a combination of Olga Perederieieva answer

Please refer to this website for a detailed explanation and sample

Header:

#include <memory>

class FridgeImpl;

class Fridge
{
public:
   DLL_EXPORT Fridge();
   DLL_EXPORT ~Fridge();
   DLL_EXPORT void coolDown();
private:
   std::unique_ptr<FridgeImpl> impl_;
};

Implementation:

#include "Engine.h"
#include "Fridge.h"

class FridgeImpl
{
public:
   void coolDown()
   {
      /* ... */
   }
private:
   Engine engine_;
};

Fridge::Fridge() : impl_(new FridgeImpl) {}

Fridge::~Fridge() = default;
Zimmer answered 3/1, 2019 at 18:15 Comment(1)
no url on the linkSaharan
C
19

That is a very common issue with DLL classes, that use templates from std.

Why does it happen?

Reason is very simple: standard specifies only guarantees, limitations and requirements. So you can be sure, that every C++ 11 compiler will provide std::unique_ptr, that looks and works as described on this page. But everything else is implementation dependent.

The main problem is, that different implementations may (and usually, will) use a totally different structure for particular types. They use additional helper variables, different layout and so on. This may differ even between two versions of the same compiler. So if client code touches in any way member variables of your class, you need to provide DLL interface for them. That applies recursively to all types used by dllexported class.

You may want to read this article on MSDN, that describes this problem with containers in mind.

This problem can be simplified to the following:

  • If client code has no access to your data, disable this warning.
  • If you have members, that are intended to use by client code, create wrapper, that is dllexported or use additional indirection with dllexported methods.
  • Usually, you can use PIMPL to hide non-DLL types, but in your case it is not applicable, since you use non-exportable type to actually implement PIMPL.

Further reading:

Cogwheel answered 19/8, 2015 at 14:40 Comment(21)
Or, write the 30-line simple_unique_ptr and punt? Or is it 60? It isn't many for a bare-bones RAII pointer-owner.Yt
Yes, that is probably the best solution. When we started to design our rendering engine (which was designed to be a set of DLL modules), we began from implementing custom containers, strings and RAII primitives. Costs: 3 days of work for 4 people. Benefits: huge performance boost. no problems with DLL interfaces.Cogwheel
what? you use hand-rolled containers instead of standard containers because of some DLL issue? usually in linux the way these things are dealt with is (1) there is no DLL_export (2) clients know that they need to use libraries built with the same compiler and standard library the whole way through, and if they don't then most likely they get linker errors, because of exactly this issue. would you seriously recommend that each DLL should ship its own private collection of containers duplicating the standard library?Palace
Custom containers? Really, why do you need more than vector. String is just vector with some sugar. Efficient maps are vector with some sugar. I guess RAII is needed.Yt
@ChrisBeck In the land of windows, DLLs are supposed to be portable (between OS versions, compiler versions, systems, etc). Restricting to being compiled with the same compiler version and quirks is considered overly restrictive. If you are shipping some collection of DLLs that make up a set of programs, that level of requirement is reasonable: otherwise, not. So you get DLLs that export COM interfaces and other loveliness.Yt
@Yakk wow... I did not realize that about DLL portability in windows. Re containers: I guess I commonly use unique_ptr, shared_ptr, vector, unordered_map (not a vector under the hood)... that's probably mostly it. but i really would not want to reimplement shared_ptr with all the bells and whistles in a one-off way for some project, like weak_ptr, make_shared, enable_shared_from_this, exception safety and make tests for it and for all the other guys etc. etc. don't forget the "secret constructor" justsoftwaresolutions.co.uk/cplusplus/…Palace
@Yakk What are you talking about? There are plenty of containers required for specific, high-performance oriented tasks not present in std. Priority trees, cascade lists, octree/kd-tree (for BSP), lock-free data structures. Also, can you simply accept, that we were able to produce more efficient replacements for some std containers, highly optimized for parallel processing? Profiling left no doubts. Standard library is not answer for everything.Cogwheel
@Yakk: also I think that its healthier if you use the standard library and when you are designing code you know that you have all the usual tools available with extremely reliable implementations where every corner case has been clearly thought out etc. if i know that I only have some particular vector and unique_ptr classes and I have strong incentives to shoehorn everything into those i think it will put bad pressure on the designPalace
@ChrisBeck shared_ptr is rarely a good idea (really), especially over something as loosely coupled as a DLL boundary. Non-flat containers are horribly slow (even linear search on an unsorted flat array does surprisingly well against advanced std containers; amortized sort flat arrays, flat hash tables, etc blow away node-based data structures). There is an idiom that you should first consider vector, and if vector doesn't work you should reconsider your problem to see if it could use vector.Yt
@Yakk Flat containers are horribly slow when it comes to ordered insertion and non ordered flat containers are horrifically slow at searching. std::vector really isn't the solution to everything.Cornelie
@MateuszGrzejek I was presuming you where just talking about std. However, you really expose kd-trees across DLL boundaries? Usually my DLLs represent systems that don't require that rich of an interface between them. I was making the point that there are 2 containers in std and one smart pointer worth the effort of reimplementing: std::vector, std::array and std::unique_ptr. Every other container should be tossed and redesigned, if you are going to that much bother of rewriting them.Yt
@Cornelie amortized sorted flat containers are only slow if you constantly add/remove elements, almost as often as you do lookup/iteration. The container doesn't have to always be sorted, it just has to be mostly sorted (and get more sorted as you poke at it). I'm sure a node-based container could be written that doesn't have the cache problems of every non-flat std container; but I was comparing a well written flat map to a std::map. There is a lot of headroom gained when you stop randomly walking memory (like node-based containers do on most operations).Yt
in the past when i have cross compiled for windows using MinGW, there is an option to automatically statically link libc / libstdc++ into the binary so that I do not have to provide it to the users / worry that the users have a competing version. is there a similar option for msvc to link the msvc standard library implementation automatically into my library, like in the case of the question?Palace
@Yakk No, I did not expose those as public/accessible/explicitly modifiable variables. I was just giving examples of what std lacks of. And nothing strange here - language cannot provide tools for solving every problem. I mean, I think that "use boost" or "use vector/string" answers became so common, that sometimes they are used to answer questions, for which they are not really applicable.Cogwheel
@ChrisBeck yes, you can link the "runtime library" statically or as a dll in the project c++/code generation propertiesZimmer
@ChrisBeck What you want is Project properties -> C/C++ -> Code Generation -> Runtime Library. /MT and MTd refer to static linking. Note, that this may cause problems if you produce both libraries (.lib/.dll) and .exe - you will end up with multiple copies of CRT symbols.Cogwheel
is that an alternate way to resolve this warning, or does the warning still happen anyways?Palace
ok i think i see now why that is not a good question. the issue is that you just want your interface to your library to be very simple, and the interface should ideally not have complex external dependencies. so you really don't want to use std:: anything, or anything that is not extremely stable, among the exported symbols. (agree / disagree?)Palace
@ChrisBeck you cannot statically link the runtime to a dll. And yes, as I mentioned in the description, using a raw pointer eliminates this warning and work just fine.Zimmer
kittikun: I think what I would do is just go back to raw pointer, only in the DLL exported classes (I am sorry to say) that seems like the simplest solutionPalace
@ChrisBeck Actually, I was wrong, just say the warning for the raw pointer! Well i'll just disable the warning then..Zimmer
F
17

Instead of exporting the entire class, you could export public methods only:

class Framework
{
    Framework(const Framework&) = delete;
    Framework& operator=(const Framework&) = delete;
    Framework(Framework&&) = delete;
    Framework& operator=(Framework&&) = delete;

public:
    EXPORT_API Framework();
    EXPORT_API ~Framework();

private:
    std::unique_ptr<FrameworkImpl> impl_;
};
Fluorine answered 8/6, 2017 at 23:34 Comment(0)
Z
0

The solution was to declare the constructor/destructor after the impl declaration and a combination of Olga Perederieieva answer

Please refer to this website for a detailed explanation and sample

Header:

#include <memory>

class FridgeImpl;

class Fridge
{
public:
   DLL_EXPORT Fridge();
   DLL_EXPORT ~Fridge();
   DLL_EXPORT void coolDown();
private:
   std::unique_ptr<FridgeImpl> impl_;
};

Implementation:

#include "Engine.h"
#include "Fridge.h"

class FridgeImpl
{
public:
   void coolDown()
   {
      /* ... */
   }
private:
   Engine engine_;
};

Fridge::Fridge() : impl_(new FridgeImpl) {}

Fridge::~Fridge() = default;
Zimmer answered 3/1, 2019 at 18:15 Comment(1)
no url on the linkSaharan

© 2022 - 2024 — McMap. All rights reserved.