12

I have a piece of C++ code for which I am not sure whether it is correct or not. Consider the following code.

#include <memory>
#include <vector>
#include <map>

using namespace std;

int main(int argc, char* argv[])
{
    vector<map<int, unique_ptr<int>>> v;
    v.resize(5);

    return EXIT_SUCCESS;
}

The GCC compiles this code without a problem. The Intel compiler (version 19), however, stops with an error:

/usr/local/ [...] /include/c++/7.3.0/ext/new_allocator.h(136): error: function "std::pair<_T1, _T2>::pair(const std::pair<_T1, _T2> &) [with _T1=const int, _T2=std::unique_ptr<int, std::default_delete<int>>]" (declared at line 292 of "/usr/local/ [...] /include/c++/7.3.0/bits/stl_pair.h") cannot be referenced -- it is a deleted function
    { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
                            ^
      detected during:

[...]

instantiation of "void std::vector<_Tp, _Alloc>::resize(std::vector<_Tp, _Alloc>::size_type={std::size_t={unsigned long}}) [with _Tp=std::map<int, std::unique_ptr<int, std::default_delete<int>>, std::less<int>, std::allocator<std::pair<const int, std::unique_ptr<int, std::default_delete<int>>>>>, _Alloc=std::allocator<std::map<int, std::unique_ptr<int, std::default_delete<int>>, std::less<int>, std::allocator<std::pair<const int, std::unique_ptr<int, std::default_delete<int>>>>>>]"
                  at line 10 of "program.cpp"

Both compilers compile the following code without a problem.

#include <memory>
#include <vector>
#include <map>

using namespace std;

int main(int argc, char* argv[])
{
    vector<unique_ptr<int>> v;
    v.resize(5);

    return EXIT_SUCCESS;
}

The first code fails with the Intel compiler, because it tries to create a copy of the unique_ptr, which only defines a move constructor. I am, however, not sure whether the first program is a legal C++ program.

I would like to know whether the first code is wrong or whether there is a bug in the Intel compiler. And if the first code is wrong, why is the second one correct? Or is the second one also wrong?

SergeyA
  • 61,605
  • 5
  • 78
  • 137
H. Rittich
  • 814
  • 7
  • 15
  • Looks like an intel bug to me. – SergeyA Nov 28 '18 at 20:38
  • 1
    Most likely intel doesn't mark `pair`'s move constructor as `noexcept`. I can recal if this is a QOI issue or acxtually a bug. – NathanOliver Nov 28 '18 at 20:39
  • 1
    @NathanOliver Intel uses GCC's STL (it doesn't have one), so it would use whatever is available to GCC – SergeyA Nov 28 '18 at 20:40
  • 3
    @SergeyA but `noexcept` and `std::is_nothrow_move_constructible` require compiler support, and they seem to fail for map of unique pointers on ICC – Piotr Skotnicki Nov 28 '18 at 20:44
  • @PiotrSkotnicki which brings us back to the original statement - it is a bug in Intel. My message was that it is not `std::pair` constructor, which is part of STL. – SergeyA Nov 28 '18 at 20:45
  • libstdc++ 7.3.0 would use `map(map&&) = default;` to declare a move-constructor. probably icc auto-generates no-`noexcept` version, forcing a vector to make a copy on relocation – Piotr Skotnicki Nov 28 '18 at 21:10
  • 2
    MSVC 18.00.40629 (from 2017 15.9.2) also has this problem – Jaka Nov 28 '18 at 21:28
  • Standard doesn't require map to be noexcept, so Intel is right, standard is flawed in this case. – JVApen Nov 28 '18 at 21:39
  • @JVApen this also fails with deque, set and list on MSVC does are those also not required to be noexcept? – Jaka Nov 28 '18 at 21:48
  • At least in MSVC's STL vector is noexcept default constructible if the contained type is nothrow default constructible. map, set, list and deque are not. tuple and pair again are... – Jaka Nov 28 '18 at 21:55
  • @Jaka: I don't know, I know I worked around this specific bug after finding a report it was correct behavior – JVApen Nov 28 '18 at 22:06
  • [Compiles](https://godbolt.org/z/GFl5fB) with ICC 18. ICC 19 also [fails to compile](https://godbolt.org/z/HqQPHS) with `reserve` instead of `resize` - I wonder if that is still correct behaviour. – cantordust Nov 28 '18 at 22:59

1 Answers1

20

The problem originates from the following post-condition of std::vector<T>::resize, [vector.capacity]:

Remarks: If an exception is thrown other than by the move constructor of a non-CopyInsertable T there are no effects.

That is, a vector must remain unchanged if relocation fails. One of the reasons relocation may fail is due to an exception, specifically, when a copy or move constructor, used for shifting elements from an old storage to a new one, throws an exception.

Does copying elements change the original storage in any way? No1. Does moving elements change the original storage? Yes. Which operation is more efficient? Moving. Can a vector always prefer moving to copying? Not always.

If a move constructor can throw an exception, there's no possibility to restore the original content of the old storage, because an attempt to move the already shifted elements back to the old chunk may fail again. In such a case, a vector will use a move constructor to relocate its elements from the old storage to a new one only if that move constructor guarantees it will not throw an exception (or a move constructor is the only option when a copy constructor is not available). How does a function promise it will not throw an exception? One will be annotated with the noexcept specifier and tested with the noexcept operator.

Testing the below code with icc:

std::map<int, std::unique_ptr<int>> m;
static_assert(noexcept(std::map<int, std::unique_ptr<int>>(std::move(m))), "!");

fails on the assertion. This means that m is not nothrow-MoveConstructible.

Does the standard require it to be noexcept? [map.overview]:

// [map.cons], construct/copy/destroy:
map(const map& x);
map(map&& x);

std::map is both Move- and CopyConstructible. Neither is required not to throw an exception.

However, an implementation is allowed to provide this guarantee {{citation needed}}. Your code uses the following definition:

map(map&&) = default;

Is an implicitly generated move constructor required to be noexcept? [except.spec]:

An inheriting constructor ([class.inhctor]) and an implicitly declared special member function (Clause [special]) have an exception-specification. If f is an inheriting constructor or an implicitly declared default constructor, copy constructor, move constructor, destructor, copy assignment operator, or move assignment operator, its implicit exception-specification specifies the type-id T if and only if T is allowed by the exception-specification of a function directly invoked by f's implicit definition; f allows all exceptions if any function it directly invokes allows all exceptions, and f has the exception-specification noexcept(true) if every function it directly invokes allows no exceptions.

At this point, it's hard to say whether the implicitly generated by icc move constructor should be noexcept or not. Either way, std::map itself was not required to be nothrow-MoveConstructible, so it's more a quality of implementation issue (implementation of the library or implementation of implicit generation of constructors) and icc gets away with it regardless of this being an actual bug or not.

Eventually, std::vector will fall back to using the safer option which is a copy constructor to relocate its elements (maps of unique pointers), but since std::unique_ptr is not CopyConstructible, an error is reported.

On the other hand, std::unique_ptr's move constructor is required to be noexcept, [unique.ptr.single.ctor]:

unique_ptr(unique_ptr&& u) noexcept;

A vector of unique pointers can safely move its elements when relocation is required.


In a newer version of stl_map.h there's the following user-provided definition of map's move constructor:

map(map&& __x)
  noexcept(is_nothrow_copy_constructible<_Compare>::value)
  : _M_t(std::move(__x._M_t)) { }

which explicitly makes noexcept dependent only on whether or not copying a comparator throws.


1 Technically, a copy constructor accepting a non-const l-value reference can change the original object, e.g., std::auto_ptr, but MoveInsertable requires vector elements to be constructible from r-values, that cannot bind to non-const l-value references.

Piotr Skotnicki
  • 46,953
  • 7
  • 118
  • 160
  • Great answer. Thank you. – H. Rittich Nov 29 '18 at 12:03
  • It’s important to note *why* the library is specified this way despite this misbehavior: the main reason is the support for **incomplete** types in containers like `std::map`, which precludes making them non-copyable if their element type is. – Davis Herring Jun 30 '20 at 15:05