8

Consider the following code:

#include <memory>
#include <vector>

class A
{
private:
  std::vector<std::unique_ptr<int>> _vals;
};

int main()
{
  A a;
  //A a2(a);
  return 0;
}

Compiler A compiles this without issue unless I uncomment out the line A a2(a); at which point it complains about the copy constructor for std::unique_ptr being deleted, and therefore I can't copy construct A. Compiler B, however, makes that complaint even if I leave that line commented out. That is, compiler A only generates an implicitly defined copy constructor when I actually try to use it, whereas compiler B does so unconditionally. Which one is correct? Note that if I were to have used std::unique_ptr<int> _vals; instead of std::vector<std::unique_ptr<int>> _vals; both compilers correctly implicitly delete both copy constructor and assignment operator (std::unique_ptr has a explicitly deleted copy constructor, while std::vector does not).

(Note: Getting the code to compile in compiler B is easy enough - just explicitly delete the copy constructor and assignment operator, and it works correctly. That isn't the point of the question; it is to understand the correct behavior.)

T.C.
  • 133,968
  • 17
  • 288
  • 421
R_Kapp
  • 2,818
  • 1
  • 18
  • 32
  • [This copy constructor reference](https://en.cppreference.com/w/cpp/language/copy_constructor) might help you. – Some programmer dude Jun 21 '18 at 17:27
  • 2
    Which compiler is compiler B? Feel free to use the real name of the compilers you are using. – François Andrieux Jun 21 '18 at 17:28
  • Out of curiosity, which are compilers A and B? – Borgleader Jun 21 '18 at 17:28
  • Compiler A is GNU 7.1, compiler B is various versions of the Intel compiler – R_Kapp Jun 21 '18 at 17:28
  • 2
    I can't repro with any intel compiler [here](https://godbolt.org/g/fUi8T4)... Are you using some very old version? – HolyBlackCat Jun 21 '18 at 17:33
  • 1
    The compiler should not give you an error on a comment. If it does it has a bug. – NathanOliver Jun 21 '18 at 17:34
  • @HolyBlackCat: Now that you mentioned that, it only appears to happen on the Intel compiler on Windows - when I try it using the Intel compiler on Linux, it works correctly... That is a bit bizarre to me. I am using the latest version of the compiler (18.3) that is out – R_Kapp Jun 21 '18 at 17:35
  • 1
    Please use https://gcc.godbolt.org and share the link which demonstrates error with icc. Generating error with commented out line is a showstopper bug for any decent compiler. – SergeyA Jun 21 '18 at 17:36
  • @R_Kapp Are you specifying the C++ standard version explicitly or allowing the compiler to choose? – Zan Lynx Jun 21 '18 at 17:39
  • @NathanOliver: It's not generating the error on the comment, but on the implicitly generated copy constructor. If I don't uncomment the particular line in question, I never actually call the copy constructor, and GNU 7.1 doesn't generate one. Intel, though (only on Windows, and only when I declare `class A` a DLL export), generates a copy constructor regardless of whether I need it. – R_Kapp Jun 21 '18 at 17:39
  • @SergeyA: As I mentioned in another comment, it's only on Windows, and only when I explicitly list `class A` as a DLL export (via, e.g., `class __declspec(dllexport) A`) that this happens. That might have been a clue to me... – R_Kapp Jun 21 '18 at 17:40
  • @ZanLynx: I explicitly specify `/Qstd=c++11` (Intel Windows version of `std=c++11`) – R_Kapp Jun 21 '18 at 17:40
  • 9
    @R_Kapp That is information that should be in the question. Your "mvce" doesn't represent what you are actually doing. – NathanOliver Jun 21 '18 at 17:43
  • 1
    @R_Kapp if you are looking for any sensible conversation, you need to provide all those details in MCVE. – SergeyA Jun 21 '18 at 17:43
  • @SergeyA: This is why I left it as "compiler A" and "compiler B" in the question - I'm not looking to see if anyone else can reproduce the issue, I'm looking to see which one is right. – R_Kapp Jun 21 '18 at 17:44
  • @R_Kapp MCVE should be provided with 'who is right' questions as well. I am still not convinced your analysis is correct, like I said before, this would be a showstopper bug with the compiler. – SergeyA Jun 21 '18 at 17:46
  • @SergeyA: I guess I don't really care if anyone's "convinced" that I found this? How is that at all relevant to the question? Surely that's something that I would have to take up with Intel support? – R_Kapp Jun 21 '18 at 17:48
  • @NathanOliver: That information is tangential to the problem - it is highly relevant if and when I file a bug report against a specific compiler, but it is not relevant to whether the attached program should compile. Being on Windows or Linux (or exporting something or importing something) does not affect the C++ standard of copy constructors. – R_Kapp Jun 21 '18 at 18:01
  • 3
    Since it only happens when using `__declspec(dllexport)`, that should be part of the MCVE. (Especially if there is a chance that declaring a class for export causes its implicitly defined constructors to be instantiated for the DLL interface.) – JaMiT Jun 21 '18 at 18:03

3 Answers3

9

From [class.copy.ctor]/12:

A copy/move constructor that is defaulted and not defined as deleted is implicitly defined when it is odr-used ([basic.def.odr]), when it is needed for constant evaluation ([expr.const]), or when it is explicitly defaulted after its first declaration.

A's copy constructor is defaulted, so it's implicitly defined only when it is odr-used. A a2(a); is just such an odr-use - so it's that statement that would trigger its definition, that would make the program ill-formed. Until the copy constructor is odr-used, it should not be defined.

Compiler B is wrong to reject the program.

Barry
  • 286,269
  • 29
  • 621
  • 977
  • Your conclusion is true iff * compiler B* rejects the code as it is actually written in the question. Since the comments reveal that *__declspec(dllexport)* is also in the game, I think things might be a little less trivial. – Pixelchemist Jun 21 '18 at 20:30
3

Note: My answer is based on your comment:

[...] it's only on Windows, and only when I explicitly list class A as a DLL export (via, e.g., class __declspec(dllexport) A) that this happens. [...]

On MSDN we can learn that declaring a class dllexport makes all members exported and required a definition for all of them. I suspect the compiler generates the definitions for all non-deleted functions in order to comply with this rule.

As you can read here, std::is_copy_constructible<std::vector<std::unique_ptr<int>>>::value is actually true and I would expect the supposed mechanism (that defines the copy constructor in your case for export purposes) checks the value of this trait (or uses a similar mechanism) instead of actually checking whether it would compile. That would explain why the bahviour is correct when you use unique_ptr<T> instead of vector<unique_ptr<T>>.

The issue is thus, that std::vector actually defines the copy constructor even when it wouldn't compile.

Imho, a is_copy_constructible check is sufficient because at the point where your dllexport happens you cannot know whether the implicit function will be odr-used at the place where you use dllimport (possibly even another project). Thus, I wouldn't think of it as a bug in compiler B.

Pixelchemist
  • 24,090
  • 7
  • 47
  • 71
  • There are times though, where the compiler will inline something from a header in a different project - wouldn't this be a perfect use case for that? I.e., if I don't declare the copy constructor or odr-use it within my module, don't include at compile time - instead, inline it into any module that does odr-use it. To me, that's a perfectly valid fix (especially as an implicitly defined copy constructor should be fairly simple) - would that cause any other issues? – R_Kapp Jun 21 '18 at 20:25
  • @R_Kapp: How will the other project know whether you (or your compiler) defined the copy constructor in your `dll` or whether it should generate one "inline" if and only if there is an odr-use? If the definition is in the header file it can be inlined but if the function is implicit you cannot tell at compiler time of project `Y` whether dll `X` ships a definition or not. Since `A`s copy constructor is not implicitly deleted because that of `std::vector` isn't deleted either, an implicit definition should be generated. – Pixelchemist Jun 21 '18 at 20:30
  • That's a fair point... I suppose an easier fix would be to always inline (and not export) implicitly defined copy constructors. This way, you won't violate ODR (nothing's ever exported), and it is still only defined when it is ever actually odr-used. – R_Kapp Jun 21 '18 at 20:46
  • (Feel free to tell me I should ask this as a separate question if you'd prefer) Also, how does this work on Linux with GNU? The class is listed as "for export" there as well, and I'm not getting that function to be generated implicitly. How would that work if I were to use it in another module/project? My guess is that it would do something like what I've suggested above (i.e., inline it always if it's implicit), but there might be some specific differences between the two OSes that I'm not aware of. – R_Kapp Jun 21 '18 at 20:56
0

While I can't confirm this behavior (I do not have access to Windows compiler, and OP claims the bug happens with icc on Windows platform), taking the question at it's face value, the answer is - compiler B has a gross bug.

In particular, implicitly-declared copy constructor is defined as deleted, when ...

T has non-static data members that cannot be copied (have deleted, inaccessible, or ambiguous copy constructors);

https://en.cppreference.com/w/cpp/language/copy_constructor

Thus, conforming compiler must semantically generate deleted copy-constructor, and successfully compile the program since such constructor never called.

SergeyA
  • 61,605
  • 5
  • 78
  • 137
  • `std::vector` is copyable, though. As I mention in the question, if I replace `std::vector> _vals` with `std::unique_ptr _vals`, I don't get the issue - `std::unique_ptr` isn't copyable and therefore the copy constructor is correctly implicitly deleted. `std::vector`, which is what the compiler has, though, is copyable. – R_Kapp Jun 21 '18 at 17:43
  • @R_Kapp why do you think `std::vector` of `std::unique_ptr`s is copyable? – SergeyA Jun 21 '18 at 17:44
  • @R_Kapp: `std::vector` is not a type. `std::vector>` is not copyable. – Dietrich Epp Jun 21 '18 at 17:44
  • `std::vector` has a copy constructor defined. It is not deleted, inaccessible, or ambiguous. It so happens that the template parameter that it uses in this case (`std::unique_ptr`) is not copyable, but that doesn't change the definition of `std::vector` – R_Kapp Jun 21 '18 at 17:55
  • See [this link](https://ideone.com/X5FRld) as evidence that `std::vector>` does not match the description provided in this answer. – R_Kapp Jun 21 '18 at 20:59