0

thank you for investing your time and have a nice happy new year.

Problem

The Dummy object desctructor is called to many times.

What I expect?

Dummy desctructor should be called only after item removal or when TestMap scope is ended.

What help do I need?

  • Why my desctructor is called more than needed?
  • How can I solve this problem?
  • Does my simple array contain any more problems?
  • Are there are things that could be done better in performance perspective?

Overall opinion

If anyone spots any weird parts, please comment them - I am new at c++ - that would help me to grow.

Current Code

struct Dummy {
    std::string name;
    std::string path;
    int age;
    long distance;
    std::string haha;

    explicit Dummy() {
        std::cout << "CONSTRUCTOR DEFAULT" << std::endl;
    }

    explicit Dummy(std::string name) : name(std::move(name)) {
        haha = randomString(10);
        std::cout << "CONSTRUCTOR CUSTOM: " << haha << std::endl;
    }

    explicit operator std::string() const {
        return name;
    }

    ~Dummy() {
        std::cout << "DESTRUCTOR: " << haha << std::endl;
    }
};

template<typename T>
class TestMap {
private:
    class Item {
    private:
        std::shared_ptr<T> _object;
    public:
        Item& operator=(const T& item) {
            _object = std::make_shared<T>(item);
            return *this;
        }

        Item& operator=(const T&& item) {
            _object = std::make_shared<T>(std::move(item));
            return *this;
        }

        T& get() {
            return *_object;
        }

        inline bool isSet() const {
            return _object != nullptr;
        }
    };
private:
    Item* _values = nullptr;

    size_t _capacity = 0;
    size_t _size = 0;
protected:
    void reallocate(const size_t& newCapacity) {
        Item* newBlock = new Item[newCapacity];

        for (size_t i = 0; i < _capacity; i++)
            if (_values[i].isSet())
                newBlock[i] = std::move(_values[i]);

        delete[] _values;
        _values = newBlock;
        _capacity = newCapacity;
    }
public:
    explicit TestMap(const size_t& size = 2) {
        reallocate(size);
    }

    void set(const size_t& key, const T& value) {
        if (key > _capacity)
            reallocate(key + _capacity / 2);
        _values[key] = value;
    }

    void set(const size_t& key, const T&& value) {
        if (key > _capacity)
            reallocate(key + _capacity / 2);
        _values[key] = std::move(value);
    }

    T* get(const size_t& key) {
        if (key > _capacity)
            return nullptr;
        if (!_values[key].isSet())
            return nullptr;
        return &_values[key].get();
    }

    bool remove(const size_t& key) {
        if (key > _capacity)
            return false;
        if (_values[key].isSet()) {
            _values[key].clear();
            _size--;
        }
    }

    void clear() {
        delete[] _values;
        _size = 0;
        _capacity = 0;
        reallocate(2);
    }

    size_t size() const {
        return _size;
    }

    ~TestMap() {
        delete[] _values;
    }
};


int main() {
    TestMap<Dummy> test;
    {
        {
            Dummy abc("aaaa");
            test.set(59, abc);
            std::cout << "out of scope" << std::endl;
        }
        test.set(89, Dummy("bbbb"));
        test.set(380, Dummy("cccc"));
    }
    std::cout << "after here everything is fine" << std::endl;
    return 0;
}
Modestas
  • 41
  • 1
  • 5
  • 1
    You might want to look at the [rule of 3/5/0](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). In particular, here you have a user-defined destructor but you _don't_ have your own definitions of copy and move operations. So any time a `Dummy` gets copy-constructed or move-constructed, it'll happen silently until it gets destroyed. – Nathan Pierson Dec 31 '21 at 17:34
  • I looked through the link - I can't say that I understood everything, but I think I got the idea, I just want to confirm, if copy & move operations should be defined in a Dummy object? – Modestas Dec 31 '21 at 17:52
  • @Modestas Not necessarily. When you add them to the map, you're making copies of `Dummy`. Thus, the destructor is called once for the original (in main) and once for the copy (in the map). That is expected. You only have to add copy/move constructors/operators to `Dummy` if the default ones aren't adequate. – LHLaurini Dec 31 '21 at 17:58
  • @LHLaurini Hmm, thank you for explaining why does it happen like that. But I am still wondering - this is an array, it can contain any object or type that I don't have luxure to modify, is this behaviour normal for array? Quite interesting that I just tried std::unordered_map and the destroy behaviour was the same - So in another words maybe I misjudged that this is a problem at all? Can you calm down my worries? :) – Modestas Dec 31 '21 at 18:29
  • @Modestas Yes, that is normal (desirable, even). For example, say your element is a `std::string`. If you copy it into your container, that will call the [`std::string` copy constructor (7)](https://en.cppreference.com/w/cpp/string/basic_string/basic_string), which will create a new string, then copy the contents. You could also have a method for moving elements into your container, which will call the [`std::string` move constructor (8)](https://en.cppreference.com/w/cpp/string/basic_string/basic_string), which may simply move the internal pointer and leave the original empty. [...] – LHLaurini Dec 31 '21 at 19:03
  • @Modestas [...] However, in both cases, the destructor will be called once for all created elements, so `std::string` can free the internal buffer (or not, in case it's empty). – LHLaurini Dec 31 '21 at 19:05
  • Put another way - every object that is constructed will eventually be destroyed. Besides the two constructors that print something, `Dummy` also has an implicit, compiler-generated copy constructor that doesn't print anything. This is why it appears on the surface that there are more destructor calls than constructor calls - there are in fact constructor calls that you don't see. You could add a user-defined copy constructor to `Dummy` that prints something, just so you can observe it being called; then the number of constructors and destructors should balance. – Igor Tandetnik Dec 31 '21 at 21:49
  • As to where a copy could occur - here, for example: `_object = std::make_shared(item);` This allocates a new instance of `T` on the heap, that is a copy of `item`, and makes a shared pointer manage that instance. – Igor Tandetnik Dec 31 '21 at 21:51
  • `TestMap` itself violates the Rule of Three. [This program](https://godbolt.org/z/qb3vs7r4e) exhibits undefined behavior by way of double-free. All it takes is `TestMap test; TestMap test2 = test;` – Igor Tandetnik Dec 31 '21 at 22:12

0 Answers0