-1

I have such program:

#include <iostream>
#include <string>
#include <vector>
#include <algorithm>

class no_object : public std::exception
{
    protected:
        std::string mMsg;
    public:
        no_object(const char* msg) : mMsg(msg) {}
        virtual ~no_object() noexcept override {}
        virtual const char* what() const noexcept override { return mMsg.c_str(); }
};

using namespace std;

class Object {
public:
    Object(const string info) : information(info) { cout << "Object constructor!" << endl; }
    ~Object() { cout << "Object destructor!" << endl; }
    Object(const Object& obj) { information = obj.information; cout << "Copy constructor!" << endl; }
    void setInformation() {  }
    string getInformation() const { return information; }
private:
    string information;
};

class Storage {
public:
    Storage(const size_t width) {
        objs = static_cast<Object*>(malloc(sizeof(Object) * width));

        if (objs == NULL)
            throw std::bad_alloc();

        lastPointer = objs + sizeof (Object) * (width - 1);
    }

    void storeObject(Object& obj, size_t index) {
        if (isIndexOutOfRange(index))
            throw std::out_of_range("Oops, index is out of range!");

        availableIndexes.push_back(index);
        objs[index] = obj;
    }

    Object& getObjectAtIndex(size_t index) const {
        if (isIndexOutOfRange(index))
            throw std::out_of_range("Oops, index is out of range!");

        auto it = find(availableIndexes.begin(), availableIndexes.end(), index);
        if (it == availableIndexes.end())
            throw no_object("Oops, the object for this index is not set!");

        return objs[index];
    }

    ~Storage() {
        free(objs);
    }
private:
    bool isIndexOutOfRange(size_t index) const noexcept {
        Object* indexPointer = objs + sizeof (Object) * index;

        if (indexPointer > lastPointer)
            return true;

        return false;
    }

    vector<size_t> availableIndexes;

    Object* objs;
    Object* lastPointer;
};

int main()
{
    Storage storage(3);
    {
        cout << "1" << endl;
        Object obj = Object("lambo");
        cout << "2" << endl;
        Object& objRef = obj;
        cout << "3" << endl;
        storage.storeObject(objRef, 2);
    }

    cout << "4" << endl;
    Object savedObject = storage.getObjectAtIndex(2);
    cout << "Information from stored object is " << savedObject.getInformation() << endl;

    return 0;
}

Interesting thing is that I have next output:

1
Object constructor!
2
3
Object destructor!
4
Copy constructor!
Information from stored object is lambo
Object destructor!

This program stores references to objects and then we can get them. As I know after object to which reference points to is deleted, the reference become unavailable and it points to garbage.

Here are my questions:
1. Can I use the reference in such way. Is it safe?
2. Why copy constructor is called?
3. Do exist any other problems in my code?
4. How to fix this program to be correct?

Thanks in advance.

Oleg
  • 1,027
  • 1
  • 8
  • 18
  • 3
    when deleted it does become unvailable in the sense of "you are not allowed to use your hands when you play football". You can still use your hand when playing football, its just against the rules – 463035818_is_not_an_ai Oct 29 '18 at 11:02
  • 2
    Please try to provide a [MCVE](https://stackoverflow.com/help/mcve) next time. Things like `storeObject`, `getObjectAtIndex` or `getObjectAtIndex` are not related to the question at all. Please omit anything that distracts from the actual problem – hellow Oct 29 '18 at 11:04
  • This causes undefined behaviour, which means *anything can happen*. Your expectations of behaviour were incorrect – M.M Oct 29 '18 at 11:04
  • Things don't work this way. C++ relies on programmer to ensure that object is alive and is in valid state when dereferencing pointer o reference. Language provides no means to figure out whether pointer or reference still refer to the valid object. – user7860670 Oct 29 '18 at 11:08
  • I'm more concerned about your use of `malloc` in C++. What's wrong with `objs = new Object[width]` and `delete[] objs`? – Nelfeal Oct 29 '18 at 11:16
  • @Nefeal ...and whats wrong with `objs = std::vector(width);` ? – 463035818_is_not_an_ai Oct 29 '18 at 11:17
  • @user463035818 That would be another step, but I'm assuming OP wants to manage memory manually, hence the class name "Storage". – Nelfeal Oct 29 '18 at 11:19
  • Nelfeal, I used malloc because it does not create objects when allocating space. When I use new Object[] it will create all objects in the array which I do not need. – Oleg Oct 29 '18 at 11:20
  • 1
    @Oleg You do need to create those objects before you copy-assign to them (well not really, it's just undefined behavior not to). See user2079303's answer. If you want to stick with `malloc`, you should at least use a placement-new before the copy-assignment. You can also look at [`std::aligned_storage`](https://en.cppreference.com/w/cpp/types/aligned_storage) instead of `malloc`. – Nelfeal Oct 29 '18 at 11:22
  • 1
    "He wasn't dead, your honour! He was right there and I could poke him and he still had his clothes and wallet and everything, so he must have been alive." – molbdnilo Oct 29 '18 at 11:25
  • @Oleg *I used malloc because it does not create objects when allocating space.* -- This is not the way you do this in C++. Memory set aside for objects in this fashion is accomplished by using [placement-new](https://stackoverflow.com/questions/222557/what-uses-are-there-for-placement-new), which your code fails to utilize. You can't simply manually allocate memory using `malloc`, stick an object in that space by using `=` or `memcpy`, and presto, there is an object. Even though doing so may seem to be correct, it isn't. Use the proper constructs (placement-new) to do this. – PaulMcKenzie Oct 29 '18 at 11:49

1 Answers1

2

This program stores references to objects and then we can get them

It does not store references. It stores objects.

  1. Can I use the reference in such way. Is it safe?

The way that you use references in your program is safe. None of your references exist longer than the lifetime of the referred object.

  1. Why copy constructor is called?

Because the Object savedObject = storage.getObjectAtIndex(2); is copy initialization.

  1. Do exist any other problems in my code?

You allocate a block of memory using malloc without constructing any Object instances into the memory. Then on the line objs[index] = obj; you copy-assign the Object instance that is in the memory... except there is no Object instance, since you never constructed any. As a consequence, the behaviour of your program is undefined.

Also, Storage is unsafe, because if you make copies of it (accidentally or on purpose), it will attempt to free the same allocation twice. The behaviour of that would be undefined.

To fix both issues, use std::vector instead of attempting manual memory management.

Also, lastPointer = objs + sizeof (Object) * (width - 1); is way outside the bounds of the allocated memory. You may have overlooked the fact that pointer arithmetic works by increments of the object size. Further multiplying with sizeof (Object) makes no sense in this context.

eerorika
  • 232,697
  • 12
  • 197
  • 326