what is wrong here??? dynamic array of chars

I am trying to implement the simple strcat() function by using pointers.
It states 'Access violation writing location' error, and I don't understand why...
I also tried to implement it by not using indexes (such as here), but by using
the pointers themselves (e.g, t++, s++, etc.), and of course, the same problem
occurs again... see // comment at the line the problem occurs.
thanks in advance.

Code:

---

#include <iostream>
using namespace std;

void strcat(char* t, char* s);

void main(){
        char* t, *s;
        t=new char[200];
        s=new char[100];
        if (t==NULL || s==NULL)
                exit(1);
        else
        {
                t="Hello world";
                s="avi";
                strcat(t,s);
                cout<<t;
        }       
        delete []t;
        delete []s;
}

//Ans. 3 (a)
void strcat(char* t, char* s){
        int iRead, iWrite;
        iRead=0;
        iWrite=0;
        while (t[iWrite]!='\0')       
                iWrite++;       

        while (s[iRead]!='\0')
        {                                       
                t[iWrite]=s[iRead];          //problem is here
                iRead++;
                iWrite++;
        }       
        t[iWrite]='\0';
}

---

t is pointing to a constant literal (t="Hello world";). You cannot change these. It would also crash because you're trying to free it with delete.

Not to mention you completely change the point to something new. In other words, you allocated memory is now lost.
So 1) you can't free it and 2) string literals are const, so you get an access violation trying to modify any of the data and 3) there's not enough space.
There's a reason why we avoid using C-style strings in C++.
To copy a string into an allocated buffer, use strcpy.

Thanks, people...
Now it is working and I don't understand why (the idea of constant vs. cin... it's stupid)...
here's the working version...

Code:

---

void main(){
        char *t, *s;
        t=new char[200];
        s=new char[100];
        if (t==NULL || s==NULL)
                exit(1);
        else
        {
                cin>>t;    //here's the correction
                cin>>s;    //here's the correction
                strcat(t,s);
                cout<<t;
        }       
        delete []t;
        delete []s;
}

---

No, this is bad too. It's a C way. It only increases the chance for buffer overruns and stops at whitespace.
Better to use cin.get, or even better use std::string and std::getline instead.

You can also do it your old way by doing:

Code:

---

                strcpy(t, "Hello world");
                strcpy(s, "avi");

---

Again, however, I must note that this is insecure, especially in C++ since you do no checking if there's enough room in the buffer.
The C++ way is much better and easier:

Code:

---

std::string str1 = "Hello world";
std::string str2 = "avi";
str1 += str2;

---

Quote:

---

Originally Posted by lionheart

Thanks, people...
Now it is working and I don't understand why (the idea of constant vs. cin... it's stupid)...
here's the working version...

Code:

---

void main(){
        char *t, *s;
        t=new char[200];
        s=new char[100];
        if (t==NULL || s==NULL)
                exit(1);
        else
        {
                cin>>t;    //here's the correction
                cin>>s;    //here's the correction
                strcat(t,s);
                cout<<t;
        }       
        delete []t;
        delete []s;
}

---

---

What's the idea of constant vs cin? You can use string literals, you just have to COPY it into the char array using strncpy or in the constructor of the char array.

Code:

---

void main()
{
    char t[200], s[100];  //why use dynamic allocation?
    strncpy(t, "Hello World", sizeof(t)); //copy "Hello World" to t
    strncpy(s, "avi", sizeof(s));  //Copy "avi" to s
    strcat(t,s);    //call that function you made
    printf("%s\n", t);  //display results
}

---

You may also copy string literals like this:

Code:

---

char t[] = "Hello World"

---

It only works in the constructor and the resulting size of the array will only be as long as the string + 1 so you couldnt copy anything else to it

Strncpy has its own quirks, however.
If the buffer is not large enough to hold the data you copy, it will copy sizeof(t) characters, but it won't put a null char, so you will get a non-null terminated string and therefore C string functions will mostly likely crash the application.
There's just no safe way of doing it in today's C world without a lot of additional checks.

Now that I see it, don't use void main:
http://cpwiki.sourceforge.net/Void_main

But I'm using C (and don't even ask why...), and I'm sorry that I didn't let you know of the assumption: the size of 's' cannot be over the physical size of 't'...

...and the problem with the constructor is that it is not dynamic (at least for my own purposes)...

Many thanks of the discussion...

OK, I'm confused here.
I can swear this was in the C++ section some time ago.
Furthermore, you ARE using C++ with new/delete, cin/cout.
So which is it here - C or C++?
If it's C, you should replace new/delete with malloc/free, and cin/cout with fgets/printf,
and if it's C++, you should probably use std::string and std::getline.

I also found an additional problem:

Code:

---

        if (t==NULL || s==NULL)
                exit(1);

---

If this condition is true, you never free your allocation buffers.

Quote:

---

Originally Posted by lionheart

the size of 's' cannot be over the physical size of 't'...

---

But even with this argument, even if t is just +1 greater than s, you can still get an overrun. And you're going to make the mistake someday. So it's critical that you make checks to avoid buffer overruns, so it won't come back and bite you one day, somewhere in the code.

By this:"the size of 's' cannot be over the physical size of 't'..."
I meant the size of 's + t' cannot go over the physical size of 't'... (sorry again)...

And I used this thread since it is "c programming" thread...

Thanks for the if (t==NULL || s==NULL) part... I only noticed it now... I have corrected the mistake

I also never check the result of malloc() or new. I dont know if that's good practice or not but generally if malloc or new fails, than you have bigger problems and your whole program is going to fail wether you check it or not.

EDIT: Unless its for HUGE allocations

Well, you're still using C++ code (cin/cout/new/delete).
So if you're using C, then how come you're using C++ code?

Quote:

---

Originally Posted by 39ster

I also never check the result of malloc() or new. I dont know if that's good practice or not but generally if malloc or new fails, than you have bigger problems and your whole program is going to fail wether you check it or not.
EDIT: Unless its for HUGE allocations

---

Yes, it's bad practice not to check since if it fails, you may attempt to access an invalid pointer (or in the case of C++, it will throw an exception).
It's better to fail with some elegant error or try to recover than crash.

The correction of the checkup is:

Code:

---

        if (t==NULL && s!=NULL)
        {
                delete s;
                exit(1);
        }
        else if (t!=NULL && s==NULL)
        {
                delete t;
                exit (1);
        }
        else if (t==NULL && s==NULL)               
                exit (1);

---

Just for being formal...

A better way...

Code:

---

int main()
{
        char* t = malloc(200);
        char* s = malloc(100);
        if (t==NULL || s==NULL)
                goto Exit;
        else
        {
                strcpy(t, "Hello world");
                strcpy(s, "avi");
                strcat(t, s);
                printf("%s", t);
        }       

Exit:
        if (t) free(t);
        if (s) free(s);
}

---

You still haven't explained why there's C++ in your C code.

I'm implementing the function and not the void main() part,
The void main() part is being used only for output sampling...

I'm using c++ syntax, but in a "c" kind of way... (and don't ask why, I have a deadline and a small project to implement, so I'm correcting what is there to correct, rather than learning all of C++ features and syntax). Maybe in 3-4 months, I'll be sitting here reprogramming the project, but as of now I haven't got the time for being 'politically correct", if you wish...

I just hope that the function of strcat() works, that's all:

Code:

---

void strcat(char* t, char* s){
        int iRead, iWrite;
        iRead=0;
        iWrite=0;
        while (t[iWrite]!='\0')       
                iWrite++;       

        while (s[iRead]!='\0')
        {                                       
                t[iWrite]=s[iRead];          //problem is here
                iRead++;
                iWrite++;
        }       
        t[iWrite]='\0';
}

---

Since it does, I have no other problems...

Since you're implementing your own strcat function, I would do it this way:

Code:

---

bool strcat(char* dst, const char* src, uint32_t dest_length)
{
        const char* tmp = src;   
        uint32_t count_src = strlen(src);
        uint32_t count_dst = strlen(dst);
        if (count_src + count_dst + 1 > dest_length)
                return false;
        dst += count_dst;
        while (*src) *dst++ = *src++;
        *dst = 0;
        return true;
}

---

Thanks... Will be used probably in 3-4 months... and I mean many thanks.

As for my understanding of this function, it does way more than my currnent needs and I need to "downsize" purposes. I know I make it sound pretty vague. This is a mini project for current fixes.
The most important part is using the "array" way of char, meaning indexes, rather than pointers (that was our supervisor's request). In 5-6 months time, all of the bigger project will become dynamic and in a c++ kind of way (while decremanting the c syntax entirely).

And again, many thanks for your implementationg of strcat()...

Well, no, it doesn't do much.
It copies just like strcat, but it also checks that the buffer (destination) you passed is big enough to hold the data. If it isn't, it returns false, and if it it succeeds, it returns true.
But it also uses pointers instead of indexes, but that's not difficult to change.
You're still welcome to try to understand it. A debugger might help.

I don't see any problem there. It has to do with something how you are using the function.

That's exactly what I meant...
I will use it in the bigger project, but not yet (since the current request is using a "void" syntax
and 2 pointers as parameters).
I'm guessing that our supervisor already implemented his own project, and probably used the functions just as he requested.

I understand that you shouldn't use it, but you can still dissect it and learn something from it.
But... as you go deeper into C++ (if I get you correctly), you won't need strcat at all.

And as I already said, I took it and am "learning something from it"...

Nice to know that there are good people out there after all... for:
"There's the respect that's makes calamity of so long lives,
for who would bear the weeps and scorns of time?", or so they say...