Why doesn't this create a dangling reference?

Question

I'd think that the VS2019 suggestion would create a dangling reference situation, but I tested it out, and it seems to work. What is happening here?


    template<typename MessageType>
    class Queue {
      inline static std::vector<MessageType> messages;
    public:
      static bool isEmpty() {
        return messages.size() == 0;
      }

      template <typename... Args>
      static void emplace(Args&&... args) {
        messages.emplace_back(std::forward<Args>(args)...);
      }

      static MessageType pop() {
        auto const& val = messages.back();
        messages.pop_back();
        return val;
      }
    };

It looks like the last message stays alive long enough to be copied into the return value. Is this good practice?

A compiler is under no obligation to figure out every instance of undefined behavior in the compiled code. Every time it manages to alert you to undefined behavior, consider it just as an extra bonus. The fact that the compiler missed this here simply means that you can't always expect your compiler to catch all the bugs for you. It would be nice if it did, but we can't always have nice things... — Sam Varshavchik, May 20 '21 at 00:42
VS is wrong here: with those changes, that's definitely undefined behaviour. — Etienne de Martel, May 20 '21 at 00:46
You can change the code to avoid the C++ Core Guidelines related [warning](https://learn.microsoft.com/en-us/cpp/code-quality/c26820?view=msvc-160) by not using `auto` for the type: `messageType val = messages.back();`. — 1201ProgramAlarm, May 20 '21 at 00:51
`auto const& val = messages.back(); messages.pop_back(); return val;` is Undefined Behavior, since `val` refers to a destroyed object by the time `return` is reached. But `auto const val = messages.back(); messages.pop_back(); return val;` is perfectly fine, as `val` is a *copy* of the last element of the vector, so it doesn't matter if the original is destroyed or not. — Remy Lebeau, May 20 '21 at 00:56
@alfC: In the screenshot, `val` is not a reference. In the subsequent code, it is. — Mooing Duck, May 20 '21 at 02:27
@SamVarshavchik I suspected it is bad code, but I didn't rely on the compiler when I said it worked. I tested the suggested code by popping twice, then emplacing more values and I got the expected output looking at the first popped value. — ratiotile, May 20 '21 at 20:06
@Kevin `MessageType` is a template type. I use POD structs with it. — ratiotile, May 20 '21 at 20:11

Fantastic Mr Fox · Answer 1 · 2021-05-20T01:56:51.810

It looks like the last message stays alive long enough to be copied into the return value. Is this good practice?

Unfortunately, no it doesn't, and no it isn't. The return type of std::vector<T>::back is an lvalue reference. Maybe intellisense thinks it is an rvalue reference, in which case its lifetime would be extended because of the rules here.

But it is not the case for this and the usage here is undefined behaviour. This is because the item in the list to which the reference refers has been destroyed. The reason it may still work is because the memory of the item is still there, and so it can be read correctly. This is just luck (or unluckiness, if you want to be able to find these errors). If the item destroyed by pop_back held other memory then you may see a different result, like a SEGFAULT.

I'm curious why I still get the expected value when I pop and then emplace again, which should overwrite the last value. — ratiotile, May 20 '21 at 20:07

alfC · Answer 2 · 2021-05-20T03:46:40.893

I think the suggestion is in the right track, and correct but only if you interpret it in a very contrived way.

First, The only thing it is telling you is that .back() returns a reference and it doesn't need to be assigned. Of course that reference can be dangling later and actually it can depend a lot in the specific implementation of std::vector. You don't want that.

To answer your question, yes, the line .pop_back() sometimes creates a dangling reference, depending on the implementation and runtime conditions. Formally, UB in other words.

(from https://en.cppreference.com/w/cpp/container/vector/pop_back)

Iterators and references to the last element, as well as the end() iterator, are invalidated.

Second, you are going to return a copy anyway, so "something" is fishy, in agreement with the IDE suggestion. You don't want a copy (the IDE either), but also you don't want a reference. What you may want is a moved copy, very likely.

This is efficient if the Message can be moved and should be generically correct and the IDE shouldn't suggest any improvement. Worst case it makes a copy, which is the correct behavior.

      static MessageType pop() {
        auto val = std::move(messages.back());
        messages.pop_back();
        return val;
      }

In conclusion, any UB or discussion aside this is the most conceptual expression of what you want I think.

NOTE: I think auto val is better than auto const val because you then allow NRVO. I am not 100% sure.

I like your suggestion to `std::move`, but I'm pretty sure `const` doesn't effect NRVO/copy elision. — ratiotile, May 20 '21 at 20:16
@ratiotile, yes, I am not sure (as I said). Maybe I misinterpreted a recent talk I saw. — alfC, May 20 '21 at 20:30

Why doesn't this create a dangling reference?

2 Answers2