polymorphic message container [closed]

Question

Closed. This question is off-topic. It is not currently accepting answers.

---

Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review.

Closed 11 months ago.

Improve this question

Any comments/suggestions on this design? I just want to hold onto an ordered collection of messages. Each message can be one of several types.

I'm using some code analogous to this currently in a project I'm writing, and I'm having some difficulty with it. Specifically, when I try to iterate over the messages returned by getter(), I occasionally have segfaults.

This goes away if I test each pointer is a nullptr, but there shouldn't be any null pointers in this container. Unfortunately, I am not able to reproduce this behavior in this small example.

The reason I wrap the container in Holder() is because in practice, Holder() will have some custom processing methods that operate on the std::multimap it holds onto.

Regarding constraints on substitutes, I really can't think of any.

#include <iostream>
#include <map>
#include <memory>
#include <string>
class Base {
public:
 virtual ~Base() = default; 
 virtual void display() const = 0; 
};
class Derived1 : public Base {
public:
 void display() const override {
 std::cout << "Derived1 instance\n";
 }
};
class Derived2 : public Base {
public:
 void display() const override {
 std::cout << "Derived2 instance\n";
 }
};
using MMap = std::multimap<int, std::unique_ptr<Base>>;
class Holder{
public:
 Holder() = default;
 void add(std::unique_ptr<Base> b) {
 i++;
 mmap.emplace(i, std::move(b));
 };
 const MMap& getter() const {
 return mmap;
 }
 MMap mmap;
 int i;
};
int main() {
 
 Holder h;
 std::cout << h.getter().size() << std::endl;
 h.add(std::make_unique<Derived1>());
 h.add(std::make_unique<Derived2>());
 std::cout << h.getter().size() << std::endl;
 // when could `ptr` be `nullptr` here?
 for (const auto& [key, ptr] : h.getter()) {
 ptr->display();
 }
 return 0;
}

• Ξένη Γήινος: You have a good coding style, your code is concise and very readable, and I like it. But I have a nit pick. Don't use return 0 in int main(). Get rid of it. You don't need to write it, it is implicitly assumed.
• underscore_d: @ΞένηΓήινος The language making an exception for main() presumably because so many people couldn't be bothered typing it is really funny to me. It's one line in one function that generally immediately hands off control to other functions that all do need explicitly specified return values. It doesn't hurt to add one extra line that reminds one to be in good practice, rather than being a magical case for one function.
• Snowhawk: I’m voting to close this question because code is not working as intended. From the question, "when I try to iterate over the messages returned by getter(), I occasionally have segfaults" makes the question off-topic.
• indi: @underscore_d The counterpoint is that every time you add an unnecessary line to code, you force expert reviewers to stop and question why that line is there: is it serving an actual purpose, or is the coder just exercising a personal fancy? That takes expert time and attention away from other parts of the code that may have real problems. So it actually does hurt, in practice. Frustrating and wasting the time of experts in order to coddle beginners or flex one’s style opinions is a choice, but not one I’d make or recommend.
• pacmaninbw: This question is off-topic for a number of reasons. Unlike Stack Overflow we need to see real working code from a project you have written. We do not help debug code on Code Review, that is what Stack Overflow is for. We don't want to see the minimum reproducible code. Please see How do I ask a good question?

Answer 1

Overview

Your Holder class is not really useful as everything is public!

Overall, there is nothing to really review as you have not built anything. Things you should have done:

• You should have hidden the MMap type (that should not be exposed).
• Accesses to a Base& in Holder.
  No need to expose the fact that you are storing pointers.
• Provided the begin/end overrides on Holder.
  so you can use range-based loops on the Holder object that provides a Base& object.
• Internal members should be private.

Code Review:

Example Polymorphic object fine:

class Base {
class Derived1 : public Base {
class Derived2 : public Base {

---

// Why is this exposed. 
using MMap = std::multimap<int, std::unique_ptr<Base>>;

---

// Not sure why you need this.
 Holder() = default;

---

// Sure, I like the pass-by value as you get both copy/move
//supported at the same time.
 void add(std::unique_ptr<Base> b)
// But why not allow an option that builds emplace for you?
template<typename T, typename... A>
 void emplace(A...&& args) {
 {
 mmap.emplace(++i, std::make_unique<T>(std::forward<Args>(args)...));
 }

---

// Don't like this.
// You have doomed your class by exposing internal implementation details.
 const MMap& getter() const {
// What you should have done is simply define begin/end operators.
// This is enough for a V1 the allows you to use the range-based for()
// Personally I would improve that so they expose `Base&` rather than `std::unique_ptr<Base>`.
 using I = MMap::const_iterator;
 I begin() const {return mmap.cbegin();}
 I end() const {return mmap.cend();}

---

// Theses should be private
 MMap mmap;
 int i;

---

Why std::multimap?

using MMap = std::multimap<int, std::unique_ptr<Base>>;

Seems like there is only one item per index, so that would suggest map; but also, why even a map? For the simple use case, you have std::vector would have been a better option (though if things left the queue, then maybe std::deque. You have not provided enough context to show why a map, but I have a feeling that there is an underlying reason why (so I am not going to suggest a replacement).

How I would have done it:

It's been a while since I wrote an iterator (so there may be better-automated ways of doing this).

class Holder
{
 using MMap = std::multimap<int, std::unique_ptr<Base>>;
 public:
 void add(std::unique_ptr<Base> b) {
 if (!b) {
 throw std::invalid_argument("pointer unexpectedly null");
 }
 ++i;
 mmap.emplace(i, std::move(b));
 };
 template<typename T, typename... Args>
 void emplace(Args&&... args) {
 ++i;
 mmap.emplace(i, std::make_unique<T>(std::forward<Args>(args)...));
 }
 struct const_iterator
 {
 using iterator_category = MMap::iterator::iterator_category;
 using value_type = Base;
 using difference_type = std::ptrdiff_t;
 using pointer = Base*;
 using reference = Base&;
 const_iterator(MMap::const_iterator i)
 : base(i)
 {}
 Base const& operator*() const {return *(base->second);}
 const_iterator& operator++() {++base;return *this;}
 const_iterator operator++(int) {const_iterator result(base);++base;return result;}
 bool operator==(const_iterator const& rhs) const {return base == rhs.base;}
 bool operator!=(const_iterator const& rhs) const {return base != rhs.base;}
 private:
 MMap::const_iterator base;
 };
 const_iterator begin() const {return std::cbegin(mmap);}
 const_iterator end() const {return std::cend(mmap);}
 std::size_t size() const {return mmap.size();}
 private:
 MMap mmap;
 int i;
};
int main() {
 Holder h;
 std::cout << h.size() << std::endl;
 h.emplace<Derived1>();
 h.emplace<Derived2>();
 std::cout << h.size() << std::endl;
 for (auto const& v : h) {
 v.display();
 }
}

• Taylor: Another option to hide the pointers is to use std:: variant maybe
• Toby Speight: If implementing an iterator wrapper is too onerous, one could always just implement the begin() and end() families of functions to return MMap::const_iterator directly. Don't forget the c and r variants of these functions!
• indi: If you use std::ranges::cbegin(x) instead of x.cbegin() (which you always should) You don’t need the c variants anymore. I think you still need the r variants to make std::ranges::(c)rbegin(x) work, but I’d consider that a low priority addition given the existence of std::views::reverse().

Answer 2

There's a comment here that indicates uncertainty:

 // when could `ptr` be `nullptr` here?

The obvious answer is: any time after add() has been called with a null pointer as argument, since we do no checking to prevent nulls being inserted.

• indi: The answer may appear obvious in this code, but this is not the code with the error. The real code may have many other things going on that reset one of the map values after being successfully added. This is why we require real code in code reviews (and why this really wasn’t a code review question at all, but rather one more suited for Stack Overflow).
• Toby Speight: Oh yes, with everything public, there's nothing to stop any client code from nulling out any of the values.
• indi: Again, everything is public in this code, not necessarily the real code. The real code might be written with everything private, but then there’s some silly mistake in an accessor that returns a default-constructed unique_ptr rather than the one in the map. Or the code may be absolutely perfect but there’s some obscure compiler bug causing the problem (that is, the segfault is not caused by empty unique_ptrs, as assumed, but rather something like the vtable pointer being corrupted). Without seeing the real code, you can’t assume anything.. at all.
• Toby Speight: Hang on - if this isn't real code, what's it doing in review?
• indi: An excellent question, but not one I can answer, because I neither provided the code nor any review.