4

I've searched through SO and have a question about whether the following is safe or not.

Consider this vector:

std::vector<std::pair<const key, value>> vec;

Imagine that I want to swap-and-pop an element at position i with the last one. Ignore for a moment the fact that if something goes wrong the vector may be in an unsafe state. I perfectly know that but the question is more about the other operation around. Since I can't swap the two elements, I'd be tempted to do this:

auto *elem = std::addressof(vec[i]);
std::destroy_at(elem);
std::construct(alloc, elem, std::move(vec.back());
vec.pop_back();

Again, we are in an unsafe state between line 2 and 3 but let's ignore it. I'm curious to know if destroying and reconstructing the element at position i is safe for all i instead.

From my understanding, I've a couple of doubts about it:

  • When i is 0, we're somewhat playing with the same initial pointer as stored by the vector itself. Therefore, I wonder if in this case we should force the vector to std::launder it (yeah, it's not possible, I'm just enetering the in theory field here).

  • Since the pair has a const key, I guess destroying and recostructing it can lead to UB. Though, I'm not that sure about this, just my gut feeling.

skypjack
  • 49,335
  • 19
  • 95
  • 187
  • 2
    One problem is that it won't work if you're trying to remove the last elements (i.e., `i == vec.size() - 1`). You'd destroy the last element then try to construct it from itself. – 1201ProgramAlarm Oct 28 '21 at 15:39
  • Do you really need `Key` to be `const`? [vector of const doesn't work that well](https://stackoverflow.com/questions/17313062/vector-of-const-objects-giving-compile-error) – Alan Birtles Oct 28 '21 at 15:42
  • @1201ProgramAlarm yeah, it's just an example, imagine there is an `if` to skip this case. – skypjack Oct 28 '21 at 15:48
  • @AlanBirtles I don't but I do for the question. :) – skypjack Oct 28 '21 at 15:48
  • If either of lines 2 or 3 throw, you'll be hosed. Now its bad form for a destructor to throw (although its perfectly valid), and move constructors can also throw. In either case, when the vector eventually tries to access the destroyed/non created element (often to destruct it itself) there'll be issues. – Mike Vine Oct 28 '21 at 15:48
  • @MikeVine indeed, I know that and I'm fine with ignoring it for the sake of the question (as explicitly stated). I'm more interested in the other details at the moment. – skypjack Oct 28 '21 at 15:49
  • I mean, you could always do unsafe stuff with C++. Thus many would implement their own "hack"... that could break when you upgrade your compiler for instance. It's now that the std lib has features to do it in a standardized way. That way it will likely not break, thus people are less afraid to upgrade the compiler. (just my 50 cent) – JHBonarius Oct 29 '21 at 12:56

2 Answers2

2

At first, I'm only referring to this question of you with a look onto your const-members concerns and onto the issue without the context of std::vector and std::construct_at (std::construct requires the allocator further on...):

Since the pair has a const key, I guess destroying and recostructing it can lead to UB. Though, I'm not that sure about this, just my gut feeling.

Before C++20, you're right, this would be UB if old references and pointers are still on use. But as of C++20, there's a relevant change here in basic.life#8.3 to n4861/basic.life#8.3

If, after the lifetime of an object has ended and before the storage which the object occupied is reused or released, a new object is created at the storage location which the original object occupied, a pointer that pointed to the original object, a reference that referred to the original object, or the name of the original object will automatically refer to the new object and, once the lifetime of the new object has started, can be used to manipulate the new object, if the original object is transparently replaceable (see below) by the new object. An object o1 is transparently replaceable by an object o2 if

  1. the storage that o2 occupies exactly overlays the storage that o1 occupied, and
  2. o1 and o2 are of the same type (ignoring the top-level cv-qualifiers), and
  3. o1 is not a complete const object, and
  4. neither o1 nor o2 is a potentially-overlapping subobject, and
  5. either o1 and o2 are both complete objects, or o1 and o2 are direct subobjects of objects p1 and p2, respectively, and p1 is transparently replaceable by p2.

So as of these changes, you should actually be fine in general for your particular(!) example since vector elements have to be non-const, i.e. the referred objects cannot be const complete ones (subpoint 3). Keep in mind that these phrases are relevant for the case of further using old references and pointers to this reused location! A simple placement new after an explicit destruction (not de-allocation) is fine a priori, it's the further context, that's relevant for the question about UB.

Also see the discussion from

Is it possible now with the current C++ standard draft version to define a copy assignment operator for classes with const fields without UB

Taking the std::vector-context into account with std::construct_at except the zero position case:

Your concerns about constant members are actually not longer relevant here since we have an in-between layer: The allocator (of std::vector) and the promise of simple contiguous storage and the fact, that there are no "pending" references to the old memory (as long as you do not refer to your old elem pointer further on...). Otherwise, a lot of highly efficient operations on/inside the standard vector container would not be possible (emplace data into pre-allocated ranges). The standard library itself uses similar schemes for moving and insertion for instance for many containers types. It's still very ugly to circumvent the inner memory "assumptions" of an external container this way but it should be ok for all common actual library implementations of an std::vector (but surely not for containers with complex inner logic like a map!).

Secundi
  • 1,150
  • 1
  • 4
  • 12
  • So, technically speaking, if I destroyed then reconstructed `pair::first` and had no references nor pointers to it hanging around, it wouldn't be UB even in C++17. Am I right? – skypjack Oct 29 '21 at 12:47
  • @skypjack Yes. But always keep in mind, that in doubt this depends on the actual implementation of pair and vector within your used standard library implementation! – Secundi Oct 29 '21 at 13:02
  • How so? If they implement correctly the standard and the standard says that it's not an UB, it should be fine and never depend on the actual implementation. – skypjack Oct 29 '21 at 13:09
  • @SergeBallesta Yes, I'd recommend to refer to std::construct and provide the allocator from the vector, maybe that was the OP's intention anyways? – Secundi Oct 29 '21 at 13:10
  • @skypjack The standard library implementation is free in managing its internals. The standard doesn't prohibit further helper pointer usage for instance as we see a lot within common map/tree implementations. The public(!) interface of vector must fulfill several constraints + several aspects in terms of runtime complexity and memory handling. But it doesn't need to take care about all possible hacks, it can't actually. The more robust approach would be an own vector implementation in doubt. – Secundi Oct 29 '21 at 13:20
1

This is not UB but it opens pitfalls. Before C++20 all pointers and references to vec[i] are gone at destroy time so using them after the operation would be UB.

And even in C++20 pointers or references to vec[i].first still go away, because vec[i].first is a complete const object, so it is not transparently replaceable.

It is not worse than using pop_back and then push_back or emplace_back to replace the last element of a vector. But it has the exact same caveats: you have a different object at the place where a former object existed. So using a reference or pointer to anything up to C++17, or to anything that is declared as const since C++20, is explicitely UB.

The underlying reason is that optimizing compilers are fond of caching anything that is declared as const. So any trick ending in replacing a const object is only safe if you are sure to never use a direct pointer, reference or variable that refered to that damned const thing.

It is rather evident when you play with the tail of a container, because future users of that code are aware that something has disappeared. I believe that doing that in the middle of a vector is more dangerous, because it could easily mislead future maintainers or users. At least it requires a warning comment in red flashing font.

Serge Ballesta
  • 143,923
  • 11
  • 122
  • 252
  • So, you're also saying that in C++17 it would be UB if I deleted element at position 0 (given that the vector has likely a pointer to it). Do I get it correctly? – skypjack Oct 29 '21 at 14:31
  • 2
    _`vec[i].first` **is** a complete const object_ Are you sure you know what is complete object? – Language Lawyer Oct 29 '21 at 21:59