-1

I've defined a class template Vec which holds a sequence of T like vector:

template <typename T> class Vec {
    public:
        size_t size() const { return first_free - elements; }
        size_t capacity() const { return cap - elements; }
        T *begin() const { return elements; }
        T *end() const { return first_free; }
        void resize(const size_t); // something wrong in this function
        ... 
    private:
        allocator<T> alloc; //allocates the elements
        T *elements; // pointer to the first element in the array
        T *first_free; // pointer to the first free element in the array
        T *cap; // pointer to one past the end of the array
        ...
}; 

when my code calls this function, the programm will crash,

template <typename T> void Vec<T>::resize(const size_t newcap)
{
    // this part goes well
    if(newcap > capacity()) {
        auto newdata = alloc.allocate(newcap);
        auto dest = newdata;
        auto orig = elements;
        for(size_t i = 0; i != size(); ++i) 
            alloc.construct(dest++, std::move(*orig++));
        elements = newdata;
        first_free = dest;
        cap = elements + newcap;
    }
    // crash happens in the following two parts
    else if(size() <= newcap && newcap < capacity()) { // deallocate the redundant memory where no element exists
       alloc.deallocate(elements + newcap, capacity() - newcap); // this line causes crash 
       cap = elements + newcap; 
    }   
    else if(size() < newcap) { // destroy the elements and deallocate the memory
        for(auto p = first_free; p != elements + newcap; /*empty*/)
            alloc.destroy(--p);
        alloc.deallocate(elements + newcap, capacity() - newcap); // this line causes crash 
        first_free = cap = elements + newcap;
    }
}

calling code:

Vec<string> v{"aaa", "bbb", "ccc"}; // size() == 3, capacity == 3
v.resize(2); // to resize v to size() == 2, capacity == 2

I think I've made a right deallocate calling and correct pointer arithmetic. Thank you.

Vogel_guo
  • 51
  • 2
  • 8
  • 1
    I'm not sure if I understand that line correctly, but is `alloc.deallocate(...)` supposed keep track of 'in-between' memory locations? If it works in the same way that `malloc`/`free` works, then allocating a pointer `p` and then attempting to free `p + offset` is not going to work. – Joel Cornett Dec 17 '16 at 19:28
  • http://stackoverflow.com/help/mcve – xaxxon Dec 17 '16 at 19:32
  • @JoelCornett I think I have understood what you meant and recognized my fault. Thank you. – Vogel_guo Dec 17 '16 at 19:40
  • by the way, your code is not exception safe. use std::uninitialized_copy to copy atomically. – Volodymyr Boiko Dec 17 '16 at 19:46
  • @GreenTree thank you for your reminder, I still need to learn more about dynamic memory. – Vogel_guo Dec 17 '16 at 20:09
  • @Vogel_guo you should write some tests to verify that your code is working as expected. You can easily implement a mock allocator that is a template specialization of a custom type. You should pay special attention to the edge cases and error cases. – Joel Cornett Dec 17 '16 at 21:02

1 Answers1

1
alloc.deallocate(elements + newcap, capacity() - newcap);

You can't do that. Allocator are all or nothing: you either deallocate the entire block that was allocated or you leave it alone.

To shrink your allocated memory, do the same allocate-and-copy dance that you do when the new size is greater than the current capacity, but allocate the new smaller size.

Incidentally, the part marked //this part goes well has a major problem: it never deallocates the old memory block.

Pete Becker
  • 74,985
  • 8
  • 76
  • 165
  • It's very terrible that I didn't notice the code is wrong. It's very helpful for me to understand the dynamic memory. Thank you for your careful reading and pertinent explanation. – Vogel_guo Dec 17 '16 at 19:48