I have the following files for implementing a Bucket
class. However I can't destroy the _str
member in the destructor of the Bucket
s produced by the operator+
. The error that I get is:
heap corruption detected after normal block...CRT detected that the application wrote to memory after end of heap buffer
The problem always occurs after the addition of the two Bucket
objects, however I've checked during the debugging that the produced string is correct as well as the length.
Also, how can I access the _len
attribute in the operator>>
function in order to assign a new value?
Header File:
class Bucket
{
friend ostream& operator<<(ostream& os, const Bucket& c);
friend istream& operator>>(istream& is, const Bucket& c);
public:
Bucket();
Bucket(const String& str);
Bucket(const char* str);
~Bucket();
const Bucket operator+(const Bucket& s) const;
const Bucket operator+(const char* s) const;
const Bucket& operator=(const char* c);
const bool operator==(const char* str);
private:
char* _str;
int _len;
};
Source File:
Bucket::Bucket() {
_len = 0;
_str = new char[_len];
}
Bucket::Bucket(const Bucket& myBucket) {
_len = myBucket._len;
_str = new char[_len + 1];
for (int i = 0; i < _len; i++)
_str[i] = myBucket._str[i];
_str[_len] = '\0';
}
Bucket::Bucket(const char* str) {
_len = 0;
for (int i = 0; str[i] != '\0'; i++) _len++;
_str = new char[_len + 1];
for (int i = 0; i < _len; i++)
_str[i] = str[i];
_str[_len] = '\0';
}
Bucket::~Bucket() {
if (_len > 0) delete[] _str;
}
const Bucket Bucket::operator+(const Bucket& myBucket) const {
String tempBucket;
tempBucket._str = strcat(this->_str, myBucket._str);
int len = 0;
while (tempBucket._str[len] != '\0') len++;
tempBucket._str[len] = '\0';
tempBucket._len = len;
return tempBucket;
}
const Bucket Bucket::operator+(const char* str) const {
Bucket tempBucket;
int len = 0;
tempBucket._len = this->_len;
for (int i = 0; str[i] != '\0'; i++) tempBucket._len++;
tempBucket._str = strcat(tempBucket._str, str);
tempBucket._str[len] = '\0';
tempBucket._len = len;
return tempBucket;
}
const Bucket& Bucket::operator=(const char* str) {
if (this->_str == str) {
return *this;
}
_len = 0;
for (int i = 0; str[i] != '\0'; i++) _len++;
_str = new char[_len + 1];
for (int i = 0; i < _len; i++)
_str[i] = str[i];
_str[_len] = '\0';
return *this;
}
const bool Bucket::operator==(const char* str) {
int comp = strcmp(this->_str, str);
if (comp == 0) {
return true;
}
else {
return false;
}
}
ostream& operator<<(ostream& os, const Bucket& myBucket) {
os << myBucket._str;
return os;
}
istream& operator>>(istream& is, const Bucket& myBucket) {
static char buffer[40];
is >> buffer;
int len = 0;
for (size_t i = 0; buffer[i] != '\0'; i++) {
myBucket._str[i] = buffer[i];
len++;
}
myBucket._str[len++] = '\0';
return is;
}
Main:
int main()
{
Bucket b1("Hello, "); // This is deleted by the destructor
Bucket b2(b1); // This is deleted by the destructor
cout << b1 << b2 << endl;
b2 = "Dear "; // Also works fine
Bucket b3;
cout << "Enter a name: ";
cin >> b3; // Can't assign the _len attribute
cout << b2 + b3 << ","; // not destroyed
Bucket b4(" Please write this sentence after pressing enter:\n");
b2 = "We believe that ";
cout << b4 + b2 << b1 << "and " << "Goodbye " // not destroyed
<< (b1 == "Goodbye " ? "is " : "is not ") << "the same word!\n" <<
endl;
return 0;
}
I see a lot of issues with how your Bucket
code is implemented:
the 2nd parameter of operator>>
needs to be a reference to a non-const object, otherwise the operator can't modify the object while reading data from the istream
.
lack of a copy constructor and copy assignment operator, and preferably also a move constructor and move assignment operator, per the Rule of 3/5/0.
the default constructor is allocating memory with new[]
, but since _len
is 0, the destructor won't free that memory. You need to remove the check for _len > 0
when calling delete[]
.
the default constructor is not null-terminating the allocated array, as the other constructors are doing.
operator+
is returning a new Bucket
object by value (as it should be), so marking that object as const
is redundant.
Same with the bool
that operator==
returns. However, the operator
itself should be marked as const
, since it doesn't modify the Bucket
object it is called on.
both operator+
are using strcat()
incorrectly. The Bucket
overload is modifying the contents of this->str_
, which it should not be doing, and worse this->_str
hasn't been reallocated anyway to increase its capacity to append the contents of myBucket._str
. The char*
overload is making a similar mistake with tempBucket.str_
. You need to allocate a whole new char[]
array for each tempBucket
.
the Bucket
overload of operator+
is declaring tempBucket
as String
instead of as Bucket
.
operator=
is returning the modified Bucket
object by reference (as it should be), but that object should not be maarked as const
.
Bucket
does not expose a way to access its _str
member from outside code, so there is no way that the if (this->_str == str)
check in operator==
will ever be true.
operator=
is not delete[]
'ing the old _str
memory before replacing it with a new char[]
.
operator>>
is not performing any bounds checking on the buffer
it reads in. And it is not re-allocating myBucket._str
to fit the contents of buffer
. And, it is counting the null terminator in _len
, which none of the other class methods do.
With all of that said, try something more like this instead:
class Bucket
{
friend ostream& operator<<(ostream& os, const Bucket& rhs);
friend istream& operator>>(istream& is, Bucket& rhs);
public:
Bucket(size_t len = 0);
Bucket(const Bucket& src);
Bucket(Bucket&& src);
Bucket(const char* str);
Bucket(const char* str, size_t len);
~Bucket();
Bucket operator+(const Bucket& rhs) const;
Bucket operator+(const char* str) const;
/*
Bucket& operator=(const Bucket& rhs);
Bucket& operator=(Bucket&& rhs);
Bucket& operator=(const char* str);
*/
Bucket& operator=(Bucket rhs);
bool operator==(const Bucket& rhs) const;
bool operator==(const char* rhs) const;
private:
char* _str;
size_t _len;
void swap(Bucket &other);
bool equals(const char* str, size_t len) const;
Bucket concat(const char* str, size_t len) const;
};
static size_t my_strlen(const char* str) {
const char* start = str;
if (str) while (*str != '\0') ++str;
return (str - start);
}
Bucket::Bucket(size_t len) {
_len = len;
if (len > 0) {
_str = new char[len + 1];
_str[len] = '\0';
}
else {
_str = nullptr;
}
}
Bucket::Bucket(const Bucket& src)
: Bucket(src._str, src._len) { }
Bucket::Bucket(Bucket&& src) : Bucket(0) {
src.swap(*this);
}
Bucket::Bucket(const char* str)
: Bucket(str, my_strlen(str)) { }
Bucket::Bucket(const char* str, size_t len) : Bucket(len) {
if (str && len > 0) {
for(size_t i = 0; i < len; ++i) {
_str[i] = str[i];
}
}
}
Bucket::~Bucket() {
delete[] _str;
}
void Bucket::swap(Bucket &other) {
char *ptmp = _str;
_str = other._str;
other._str = ptmp;
size_t itmp = _len;
_len = other._len;
other._len = itmp;
}
bool Bucket::equals(const char* str, size_t len) const {
if (this->_len != len) return false;
for(size_t i = 0; i < len; ++i) {
if (this->_str[i] != str[i]) return false;
}
return true;
}
Bucket Bucket::concat(const char* str, size_t len) const {
Bucket tempBucket(this->_len + len);
for(size_t i = 0; i < this->_len; ++i) {
tempBucket._str[i] = this->_str[i];
}
for(size_t i = this->_len, j = 0; j < len; ++i, ++j) {
tempBucket._str[i] = str[j];
}
return tempBucket;
}
Bucket Bucket::operator+(const Bucket& rhs) const {
return concat(rhs._str, rhs._len);
}
Bucket Bucket::operator+(const char* rhs) const {
return concat(rhs, my_strlen(rhs));
}
/*
Bucket& Bucket::operator=(const Bucket& rhs) {
if (this != &rhs) {
Bucket(rhs).swap(*this);
}
return *this;
}
Bucket& Bucket::operator=(Bucket&& rhs) {
Bucket(std::move(rhs)).swap(*this);
return *this;
}
Bucket& Bucket::operator=(const char* rhs) {
Bucket(rhs).swap(*this);
return *this;
}
*/
Bucket& Bucket::operator=(Bucket rhs) {
rhs.swap(*this);
return *this;
}
bool Bucket::operator==(const Bucket& rhs) const {
return equals(rhs._str, rhs._len);
}
bool Bucket::operator==(const char* rhs) const {
return equals(rhs._str, my_strlen(rhs));
}
ostream& operator<<(ostream& os, const Bucket& rhs) {
os.write(rhs._str, rhs._len);
return os;
}
istream& operator>>(istream& is, Bucket& rhs) {
/*
string buffer;
is >> buffer;
rhs = buffer.c_str();
return is;
*/
char buffer[40];
if (!is.get(buffer, 40)) buffer[0] = '\0';
rhs = buffer;
return is;
}
That being said, if you can use standard C++ functionalities, such as std::string
, then the code becomes a whole lot simpler:
#include <string>
class Bucket
{
friend ostream& operator<<(ostream& os, const Bucket& rhs);
friend istream& operator>>(istream& is, Bucket& rhs);
public:
Bucket() = default;
Bucket(const Bucket& src) = default;
Bucket(Bucket&& src) = default;
~Bucket() = default;
Bucket(const std::string& str);
Bucket operator+(const Bucket& rhs) const;
Bucket& operator=(const Bucket& rhs) = default;
Bucket& operator=(Bucket&& rhs) = default;
bool operator==(const Bucket& rhs) const;
private:
std::string _str;
};
Bucket::Bucket(const std::string str) : _str(str) { }
Bucket Bucket::operator+(const Bucket& rhs) const {
return Bucket(this->_str + rhs._str);
}
bool Bucket::operator==(const Bucket& rhs) const {
return (this->_str == rhs._str);
}
ostream& operator<<(ostream& os, const Bucket& rhs) {
os.write << rhs._str;
return os;
}
istream& operator>>(istream& is, Bucket& rhs) {
is >> rhs._str;
return is;
}