Implement a class want to simulate std::string but get stuck when push_back it into a vector

Question

I have implemented a class String. When I push_back a String object to a Vector, the program gets stuck. Please help me review it if you're interested.

String.h

#pragma once
#include <iostream>
#include <memory>
#include <string>

class String {
public:
   String();
   String(const char *);
   String(const String &);
   String(const std::string &);
   String(String &&) noexcept;
   String &operator=(String &&) noexcept;
   ~String();
   String &operator=(const String &);
   std::string to_string() const;

   friend std::ostream &operator<<(std::ostream &os, String s);

private:
   std::pair<char *, char *>
      alloc_n_copy(char *, char *);
   void free();
   static std::allocator<char> alloc;
   char *beg;
   char *end;
   size_t length;
};

String.cpp

#include "stdafx.h"
#include <cstring>
#include "String.h"

std::allocator<char> String::alloc = std::allocator<char>();

String::String()
   : beg(nullptr), end(nullptr), length(0) {}

String::String(const char *ptr) {
   std::cout << "Const char constructor execute" << std::endl;
   const char *tmp = ptr;
   while (*tmp++ != '\0') ++length;
   beg = alloc.allocate(length);
   end = std::uninitialized_copy(ptr, tmp, beg);
}

String::String(const std::string &s) {
   std::cout << "Const string constructor execute" << std::endl;
   strcpy_s(beg, s.size(), s.c_str());
   length = s.size();
   char *tmp = beg;
   end = tmp + length;
}

String::String(const String &s) {
   std::cout << "Copy constructor execute" << std::endl;
   beg = alloc.allocate(s.length);
   end = std::uninitialized_copy(s.beg, s.end, beg);
}

std::pair<char *, char *>
String::alloc_n_copy(char *beg, char *end) {
   auto newBeg = alloc.allocate(end - beg);
   return{ newBeg, std::uninitialized_copy(beg, end, newBeg) };
}

String &String::operator=(const String &s) {
   length = s.length;
   auto newStr = alloc_n_copy(s.beg, s.end);
   free();
   beg = newStr.first;
   end = newStr.second;
   return *this;
}

String::String(String &&s) noexcept : beg(s.beg), end(s.end), length(s.length) {
   std::cout << "Move constructor execute" << std::endl;
   s.beg = s.end = nullptr;
   s.length = 0;
}

String &String::operator=(String &&s) noexcept {
   if (this != &s) {
      beg = s.beg;
      end = s.end;
      length = s.length;
      s.beg = s.end = nullptr;
      s.length = 0;
   }
   return *this;
}

void String::free() {
   while (length-- >= 0) {
      alloc.destroy(end--);
   }
   alloc.deallocate(beg, length);
}

String::~String() {
   free();
}

std::string String::to_string() const {
   std::string s(beg);
   return s;
}

std::ostream &operator<<(std::ostream &os, String s) {
   std::string str(s.beg);
   os << str;
   return os;
}

main.cpp

int main()
{
   vector<String> v;
   String s1("abc");
   String s2("def");

   v.push_back(s1);
   v.push_back(s2);

   return 0;
}

result:

Const char constructor execute
Const char constructor execute
Copy constructor execute
Move constructor execute

I don't know why the second push_back is a move construction.

And when the push_back finished, the program can't exit. Is there any resource failed to release?

Thanks

I'd recommend running the program with an address sanitizer. — chris, Oct 15 '19 at 12:30
`Operator =` is broken if self-assignment is done. It is also broken if the memory allocation fails. Use `copy / swap` instead of rewriting your copy constructor and destructor inside of your assignment operator. — PaulMcKenzie, Oct 15 '19 at 12:38
Look at your move assignment operator. What do you do if your object already has data in it? — NathanOliver, Oct 15 '19 at 12:38
`String::String(const std::string &s)` is broken; `strcpy_s(beg, s.size(), s.c_str());` tries to copy to an initialised `beg` also no space is allocated for the copy. — Richard Critten, Oct 15 '19 at 12:47
Your assignment operator should look like this: `String& operator=(String) noexcept;` and do a `swap` (which you also need to define as a friend function, so ADL works). With modern C++, you shouldn't have a `String const&` and `String&&` parameter assignment operators. — Eljay, Oct 15 '19 at 12:49

Toby Speight · Answer 1 · 2019-10-15T16:01:17.180

1

The reason that your program blocks is because your destructor never terminates:

void String::free() {
   while (length-- >= 0) {
      alloc.destroy(--end);
   }
   alloc.deallocate(beg, length);
}

Since length is an unsigned type, length >= 0 is always true. You probably don't want to be decrementing length here, before it's used as argument to alloc.deallocate(). I suggest:

void String::free() {
   while (end > beg) {
      alloc.destroy(end--);
   }
   alloc.deallocate(beg, length);
}

There are other bugs, such as failing to initialise length before using it in the char const* constructor (I don't see why you don't just use std::strlen()) and failing to allocate in the std::string constructor. I recommend using a good set of warnings (I used g++ -std=c++2a -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Weffc++) and addressing them all. The problem above was identified very easily this way.

After compiling clear of warnings, then run your code under Valgrind or other memory checker to understand some of the outstanding issues.

edited Oct 15 '19 at 16:01

answered Oct 15 '19 at 14:10

Toby Speight

27,591
48
66
103

Thanks for the comments. I will follow it. – Zhen Yang Oct 15 '19 at 14:14
It should be while( beg >= end) Right? – Zhen Yang Oct 15 '19 at 15:18
1

I haven't tested that code properly - I think it should be `beg < end` as it is, but we should be passing `--end` to `destroy()`, given that `end` points *after* the last element. As I said, you still have a few more bugs to find (and plenty of hints in comments to the question). – Toby Speight Oct 15 '19 at 15:44
Ok, I get it. The end is after the last element. – Zhen Yang Oct 15 '19 at 15:59

Implement a class want to simulate std::string but get stuck when push_back it into a vector

1 Answers1