Differences in code implemented using this pointer and a variable THIS simulating this pointer

[chikkubhai]

Why is the result different for the following set of two code snippets

Code without using this pointer

#include <string>
#include <iostream>
using namespace std;

struct X {
private:
int len;
char *ptr;
public:
int GetLen() {
return len;
}
char * GetPtr() {
return ptr;
}
X& Set(char *);
X& Cat(char *);
X& Copy(X&);
void Print();
};

X& X::Set(char *pc) {
len = strlen(pc);
ptr = new char[len];
strcpy(ptr, pc);
return *this;
}

X& X::Cat(char *pc) {
len += strlen(pc);
strcat(ptr,pc);
return *this;
}

X& X::Copy(X& x) {
Set(x.GetPtr());
return *this;
}

void X:rint() {
cout << ptr << endl;
}

int main() {
X xobj1;
xobj1.Set("abcd")
.Cat("efgh");

xobj1.Print();
X xobj2;
xobj2.Copy(xobj1)
.Cat("ijkl");

xobj2.Print();
}

Equivalent code, the THIS variable simulating the hidden use of this
pointer

#include <string>
#include <iostream>
using namespace std;

struct X {
private:
int len;
char *ptr;
public:
int GetLen (X* const THIS) {
return THIS->len;
}
char * GetPtr (X* const THIS) {
return THIS->ptr;
}
X& Set(X* const, char *);
X& Cat(X* const, char *);
X& Copy(X* const, X&);
void Print(X* const);
};

X& X::Set(X* const THIS, char *pc) {
THIS->len = strlen(pc);
THIS->ptr = new char[THIS->len];
strcpy(THIS->ptr, pc);
return *THIS;
}

X& X::Cat(X* const THIS, char *pc) {
THIS->len += strlen(pc);
strcat(THIS->ptr, pc);
return *THIS;
}

X& X::Copy(X* const THIS, X& x) {
THIS->Set(THIS, x.GetPtr(&x));
return *THIS;
}

void X:rint(X* const THIS) {
cout << THIS->ptr << endl;
}

int main() {
X xobj1;
xobj1.Set(&xobj1 , "abcd")
.Cat(&xobj1 , "efgh");

xobj1.Print(&xobj1);
X xobj2;
xobj2.Copy(&xobj2 , xobj1)
.Cat(&xobj2 , "ijkl");

xobj2.Print(&xobj2);
}


Both examples produce the following output:
abcdefgh
abcdefghijkl

They are different for some reason. Any comments would be appreciated.

 

[Kai-Uwe Bux]

Why is the result different for the following set of two code snippets

Code without using this pointer

#include <string>
#include <iostream>
using namespace std;

struct X {
private:
int len;
char *ptr;
public:
int GetLen() {
return len;
}
char * GetPtr() {
return ptr;
}
X& Set(char *);
X& Cat(char *);
X& Copy(X&);
void Print();
};

X& X::Set(char *pc) {
len = strlen(pc);
ptr = new char[len];
strcpy(ptr, pc);
return *this;
}

X& X::Cat(char *pc) {
len += strlen(pc);
strcat(ptr,pc);
return *this;
}

X& X::Copy(X& x) {
Set(x.GetPtr());
return *this;
}

void X:rint() {
cout << ptr << endl;
}

int main() {
X xobj1;
xobj1.Set("abcd")
.Cat("efgh");

xobj1.Print();
X xobj2;
xobj2.Copy(xobj1)
.Cat("ijkl");

xobj2.Print();
}

Equivalent code, the THIS variable simulating the hidden use of this
pointer

#include <string>
#include <iostream>
using namespace std;

struct X {
private:
int len;
char *ptr;
public:
int GetLen (X* const THIS) {
return THIS->len;
}
char * GetPtr (X* const THIS) {
return THIS->ptr;
}
X& Set(X* const, char *);
X& Cat(X* const, char *);
X& Copy(X* const, X&);
void Print(X* const);
};

X& X::Set(X* const THIS, char *pc) {
THIS->len = strlen(pc);
THIS->ptr = new char[THIS->len];
strcpy(THIS->ptr, pc);
return *THIS;
}

X& X::Cat(X* const THIS, char *pc) {
THIS->len += strlen(pc);
strcat(THIS->ptr, pc);
return *THIS;
}

X& X::Copy(X* const THIS, X& x) {
THIS->Set(THIS, x.GetPtr(&x));
return *THIS;
}

void X:rint(X* const THIS) {
cout << THIS->ptr << endl;
}

int main() {
X xobj1;
xobj1.Set(&xobj1 , "abcd")
.Cat(&xobj1 , "efgh");

xobj1.Print(&xobj1);
X xobj2;
xobj2.Copy(&xobj2 , xobj1)
.Cat(&xobj2 , "ijkl");

xobj2.Print(&xobj2);
}


Both examples produce the following output:
abcdefgh
abcdefghijkl

They are different for some reason. Any comments would be appreciated.

Undefined behavior (for both programs). Your code has an off-by-one error in
the allocation of the character buffer. You should use std::string.


Best

Kai-Uwe Bux

 

[chikkubhai]

I didn't quite get you, as I have already included the string class
and compiled both code without any problem.

 

[chikkubhai]

and using namespace std will take care of what you are mentioning in C+
+

 

[Kai-Uwe Bux]

chikkubhai wrote:

[too little]

Please quote enough context. This is not a chat room. Due to the news
protocol, you cannot assume that previous post of the thread are available
on the server.

I didn't quite get you, as I have already included the string class

You have included the header <string>, but you did not use it. You used
char* and the related functions. Probably, you meant to include <string.h>

and compiled both code without any problem.

That is because standard headers are allowed to pull other standard headers.
In your implementation, either <string> or <iostream> seems include
<cstring>. That is not guaranteed by the standard. Thus, your code has a
bug there, too.

As for the undefined behavior, look closely at the following lines:

X& X::Set(char *pc) {
len = strlen(pc);
ptr = new char[len];
strcpy(ptr, pc);
return *this;
}

There is your main bug.


Best

Kai-Uwe Bux

 

[chikkubhai]

I did not quite get why you felt I was in a chat room. Take it easy.
When you mentioned that I have to use std::string I replied saying I
used using namespace std which will require me not to use std::string
anymore.

Anyways, what is or where is the off by one error? I still see the
correct output as
abcdefgh

followed by

abcdefghijkl

 

[Kai-Uwe Bux]

I did not quite get why you felt I was in a chat room.

Because you behave like that. Please read the FAQ on netiquette in this
forum (especially on quoting). You are the one asking for help. Show a
little courtesy and follows local customs.

Take it easy.
When you mentioned that I have to use std::string I replied saying I
used using namespace std which will require me not to use std::string
anymore.

Such recastings of conversations are better dealt with by quoting.

Anyway, your point about std::string vs string is irrelevant since neither
appeared in the code you posted. Instead of std::string, you used char*.
When I say you should use std::string, I do not mean "std::string" instead
of "string" (and I could not have meant that, because there was no "string"
in your code), I meant use std::string instead of char*.

Anyways, what is or where is the off by o
ne error?

X& X::Set(char *pc) {
len = strlen(pc);
ptr = new char[len]; // <--- here, you are short one character
strcpy(ptr, pc);
return *this;
}


But fixing that, will still not make it work. There are much bigger issues:

#include <string>

should read

#include <iostream>
using namespace std;

struct X {
private:
  int len;
  char *ptr;
public:
  int GetLen() {
    return len;
  }
  char * GetPtr() {
    return ptr;
  }

Poor design: this allows client code to write beyond allocated memory.
Buffer overruns will come from this.

  X& Set(char *);
  X& Cat(char *);
  X& Copy(X&);
  void Print();
};

Your class does not disable copy-construction and assignment. However, it
handles neither correctly. Your Set method allocates memory that is never
freed. The class leaks memory big time.

X& X::Set(char *pc) {
  len = strlen(pc);
  ptr = new char[len];

Here is the off by 1 error.

Also, should Set() be called twice on the same object, memory allocated in
the first run is not freed.

  strcpy(ptr, pc);
  return *this;
}

X& X::Cat(char *pc) {
  len += strlen(pc);
  strcat(ptr,pc);
  return *this;
}

This is even worse: you append beyond allocated memory.

X& X::Copy(X& x) {
  Set(x.GetPtr());
  return *this;
}
void X:rint() {
  cout << ptr << endl;
}

int main() {
  X xobj1;
  xobj1.Set("abcd")
       .Cat("efgh");

  xobj1.Print();
  X xobj2;
  xobj2.Copy(xobj1)
       .Cat("ijkl");

  xobj2.Print();
}

All in all, you should not touch char*. There is no need to deal with char*
anyway since there is std::string. You should use it.

I still see the
correct output as
abcdefgh

followed by

abcdefghijkl

Undefined behavior sometimes looks correct and sometimes looks wrong.


Best

Kai-Uwe Bux

 

[chikkubhai]

I did not quite get why you felt I was in a chat room.

Because you behave like that. Please read the FAQ on netiquette in this
forum (especially on quoting). You are the one asking for help. Show a
little courtesy and follows local customs.

Take it easy.
When you mentioned that I have to use std::string I replied saying I
used using namespace std which will require me not to use std::string
anymore.

Such recastings of conversations are better dealt with by quoting.

Anyway, your point about std::string vs string is irrelevant since neither
appeared in the code you posted. Instead of std::string, you used char*.
When I say you should use std::string, I do not mean "std::string" instead
of "string" (and I could not have meant that, because there was no "string"
in your code), I meant use std::string instead of char*.

Anyways, what is or where is the off by o
ne error?

X& X::Set(char *pc) {
len = strlen(pc);
ptr = new char[len]; // <--- here, you are short one character
strcpy(ptr, pc);
return *this;
}

But fixing that, will still not make it work. There are much bigger issues:

#include <string>

should read

#include <iostream>
using namespace std;

struct X {
private:
int len;
char *ptr;
public:
int GetLen() {
return len;
}
char * GetPtr() {
return ptr;
}

Poor design: this allows client code to write beyond allocated memory.
Buffer overruns will come from this.

X& Set(char *);
X& Cat(char *);
X& Copy(X&);
void Print();
};

Your class does not disable copy-construction and assignment. However, it
handles neither correctly. Your Set method allocates memory that is never
freed. The class leaks memory big time.

X& X::Set(char *pc) {
len = strlen(pc);
ptr = new char[len];

Here is the off by 1 error.

Also, should Set() be called twice on the same object, memory allocated in
the first run is not freed.

strcpy(ptr, pc);
return *this;
}

X& X::Cat(char *pc) {
len += strlen(pc);
strcat(ptr,pc);
return *this;
}

This is even worse: you append beyond allocated memory.

X& X::Copy(X& x) {
Set(x.GetPtr());
return *this;
}
void X:rint() {
cout << ptr << endl;
}

int main() {
X xobj1;
xobj1.Set("abcd")
.Cat("efgh");

xobj1.Print();
X xobj2;
xobj2.Copy(xobj1)
.Cat("ijkl");

xobj2.Print();
}

All in all, you should not touch char*. There is no need to deal with char*
anyway since there is std::string. You should use it.

I still see the
correct output as
abcdefgh

followed by

abcdefghijkl

Undefined behavior sometimes looks correct and sometimes looks wrong.

Best

Kai-Uwe Bux

wooow, those were a lot of help/suggestions to me dude. It will take
me awhile to even understand your comments as I am not advanced and do
not have experience to be able to point out mistakes that you could.
Thanks and I appreciate your help and time.

I will learn how to recast or requote as I have never done that
previously and will surely read the netiquette on this forum soon.

 

[Kai-Uwe Bux]

I did not quite get why you felt I was in a chat room.

Because you behave like that. Please read the FAQ on netiquette in this
forum (especially on quoting). You are the one asking for help. Show a
little courtesy and follows local customs.

Take it easy.
When you mentioned that I have to use std::string I replied saying I
used using namespace std which will require me not to use std::string
anymore.

Such recastings of conversations are better dealt with by quoting.

Anyway, your point about std::string vs string is irrelevant since
neither appeared in the code you posted. Instead of std::string, you used
char*. When I say you should use std::string, I do not mean "std::string"
instead of "string" (and I could not have meant that, because there was
no "string" in your code), I meant use std::string instead of char*.

Anyways, what is or where is the off by o
ne error?

X& X::Set(char *pc) {
len = strlen(pc);
ptr = new char[len]; // <--- here, you are short one character
strcpy(ptr, pc);
return *this;
}

But fixing that, will still not make it work. There are much bigger
issues:

#include <string>

should read

#include <iostream>
using namespace std;

struct X {
private:
int len;
char *ptr;
public:
int GetLen() {
return len;
}
char * GetPtr() {
return ptr;
}

Poor design: this allows client code to write beyond allocated memory.
Buffer overruns will come from this.

X& Set(char *);
X& Cat(char *);
X& Copy(X&);
void Print();
};

Your class does not disable copy-construction and assignment. However, it
handles neither correctly. Your Set method allocates memory that is never
freed. The class leaks memory big time.

X& X::Set(char *pc) {
len = strlen(pc);
ptr = new char[len];

Here is the off by 1 error.

Also, should Set() be called twice on the same object, memory allocated
in the first run is not freed.

strcpy(ptr, pc);
return *this;
}

X& X::Cat(char *pc) {
len += strlen(pc);
strcat(ptr,pc);
return *this;
}

This is even worse: you append beyond allocated memory.

X& X::Copy(X& x) {
Set(x.GetPtr());
return *this;
}
void X:rint() {
cout << ptr << endl;
}

int main() {
X xobj1;
xobj1.Set("abcd")
.Cat("efgh");

xobj1.Print();
X xobj2;
xobj2.Copy(xobj1)
.Cat("ijkl");

xobj2.Print();
}

All in all, you should not touch char*. There is no need to deal with
char* anyway since there is std::string. You should use it.

I still see the
correct output as
abcdefgh

followed by

abcdefghijkl

Undefined behavior sometimes looks correct and sometimes looks wrong.

Best

Kai-Uwe Bux

wooow, those were a lot of help/suggestions to me dude. It will take
me awhile to even understand your comments as I am not advanced and do
not have experience to be able to point out mistakes that you could.
Thanks and I appreciate your help and time.

Ok, here is a version of your code using std::string.

#include <string>
#include <iostream>
using namespace std;

struct X {
private:

std::string data;

public:

// note the const
// we promise that calling this function will not
// modify the object.
int GetLen() const {
return data.length();
}

// note the const in the return type:
// this way, the client cannot overwrite it.
char const * GetCstring() const {
return data.c_str();
}

// note the const in the parameter:
// we promise that we will not modify the
// string passed. The compiler will check that.
X& Set(char const *);
X& Cat(char const *);


// X& Copy(X&);
// this really should be an assignment operator copy constructor.
/*
X& operator= ( X const & rhs ) {
data = other.data;
return ( *this );
}
void Print();
};
*/
// However, the compiler generates that one for free.

void Print() const;

};

X& X::Set( char const * pc) {
data = pc;
return *this;
}

X& X::Cat(char const * pc) {
data.append( pc );
return *this;
}

void X:rint() const {
cout << data << endl;
}

int main() {
X xobj1;
xobj1.Set("abcd")
.Cat("efgh");

xobj1.Print();
X xobj2;
( xobj2 = xobj1 )
.Cat("ijkl");
xobj2.Print();
}


You should do the following experiment. Run this version and you version in
a memory checked environment, e.g., use valgrind (it's a terrific tool!).


If you are in for an interesting learning experience, you can try rewriting
that class using char* instead of std::string.

E.g.:

class X {

char * data_ptr;

public:

....

};


However, it will be difficult:

a) You have to write a default constructor. The member data_ptr will
otherwise point nowhere meaningfull and the object starts off with an
inconsistent state. BadIdea(tm)

b) You have to manually manage the memory every time the length of the
string potentially changes, i.e., in the Set(), Cat() and operator=()
method.

c) You have to provide a destructor that frees the memory when an object of
type X goes out of scope.


The library class std::string takes care of all those issues behind the
scenes.


Should you decide to recode class X, you should run all your tests in
valgrind and make sure that your code is memory clean. Whenever there is a
question, post what you have, and most likely you will get some help and
code review here.

I will learn how to recast or requote as I have never done that
previously and will surely read the netiquette on this forum soon.

That's very good. I am looking forward to your postings.


Best

Kai-Uwe Bux

 

[James Kanze]

Why is the result different for the following set of two code snippets
Code without using this pointer
#include <string>
#include <iostream>
using namespace std;
struct X {
private:
int len;
char *ptr;
public:
int GetLen() {
return len;
}
char * GetPtr() {
return ptr;
}
X& Set(char *);
X& Cat(char *);
X& Copy(X&);
void Print();
};
X& X::Set(char *pc) {
len = strlen(pc);
ptr = new char[len];
strcpy(ptr, pc);
return *this;
}
X& X::Cat(char *pc) {
len += strlen(pc);
strcat(ptr,pc);
return *this;
}
X& X::Copy(X& x) {
Set(x.GetPtr());
return *this;
}
void X:rint() {
cout << ptr << endl;
}
int main() {
X xobj1;
xobj1.Set("abcd")
.Cat("efgh");
xobj1.Print();
X xobj2;
xobj2.Copy(xobj1)
.Cat("ijkl");
xobj2.Print();
}
Equivalent code, the THIS variable simulating the hidden use of this
pointer [...]
Both examples produce the following output:
abcdefgh
abcdefghijkl
They are different for some reason. Any comments would be appreciated.

Undefined behavior (for both programs). Your code has an off-by-one error in
the allocation of the character buffer. You should use std::string.

It's more than off by one; look at the function Cat.

It's also a pretty wierd class that uses dynamic memory, but
doesn't have a user defined constructor, assignment operator or
destructor.