What's wrong with this strcpy and this pointer

[earlyriser]

Hello
I'm trying to assign a value from a string via some pointer, but I got an error and I cannot figure how to solve it.
Do you have an idea? I would really help me. The problematic line is marked between *********
Thanks.

// reading a text file
#include <iostream>
#include <fstream>
#include <string>
using namespace std;


struct Cours
{
    char *sigle;
    Cours *suivant;
};

struct Etudiant
{
    char *nom;
    Etudiant *apres;
};

struct Professeur
{
    char nom [20];
    Professeur *suivant;
};


class DossierProfesseur
{        
    public:
        Professeur *tete;
        DossierProfesseur();
        ~DossierProfesseur();
        void affichage();
};


DossierProfesseur::DossierProfesseur()
{
    Professeur *PCourant, *PTete;
    string line;
    ifstream myfile ("FP.txt");
    if (myfile.is_open())
    {
        while (! myfile.eof() )
        {
            getline (myfile,line);
            cout << line << endl;
            PCourant = new Professeur;
/*****************************************************/
            strcpy (PCourant->nom, line);
/*****************************************************/
        }
        myfile.close();
    }

    else cout << "Unable to open file";
}


void main () {
 

    DossierProfesseur *dossierProfesseur;
    
    dossierProfesseur= new DossierProfesseur;
}

[iamthwee]

http://www.cplusplus.com/reference/clibrary/cstring/strcpy.html

[earlyriser]

Thanks iamthwee, but it is not helping me a lot, I was in that page before writing this post and I did what I understood:

strcpy (where-the-data-will-be, where-there-are-now)

strcpy (PCourant->nom, line);

[Ancient Dragon]

>> strcpy (PCourant->nom, line);

line is a c++ class, strcpy() wants a c style pointer. You have to use std::string's c_str method to get the pointer, like this strcpy (PCourant->nom, line.c_str());

struct Professeur
{
    char nom [20];
    Professeur *suivant;
};

Since you are writing a c++ program you should use c++ std::string class, not character arrays, unless your teacher requires you to use character arrays. Using std::string will make your life a lot easier and eleminate several problems with using strcpy() and other similar standard c string handling functions. The main problem with those functions is they will allow you to scribble all over memory by copying strings beyond the allocation of the destination buffer. Example: copy a 25 character string into that buffer that has only room for 19 characters plus null terminator. After that happens your program will likely crash at some point and you will spend hours trying to find the problem.

[iamthwee]

yeah and don't forget to clean up after with delete[], avoid void main and EOF blah blah blah.

[earlyriser]

Thanks Ancient Dragon!
It works, well, the program doesnt, but at least the strcpy works.
My teacher wants char arrays.

Thanks iamthwee!

[earlyriser]

After reading a line in a text file I need to read another line which contains an INT. I thought that the bold line in this code will be ok to retreive the data, but it doesnt.

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

struct Professeur
{
    char nom[20];
    int ancien;
};


class DossierProfesseur
{
   
        
    public:
        Professeur *tete;
        DossierProfesseur();
        ~DossierProfesseur();
};


DossierProfesseur::DossierProfesseur()
{
    Professeur *PCourant, *PTete=NULL;
    string line;
    ifstream myfile ("FP.txt");
    if (myfile.is_open())
    {
        while (! myfile.eof() )
        {
            getline (myfile,line);
            PCourant = new Professeur;
            strcpy (PCourant->nom, line.c_str());
            cout << PCourant->nom;
            [B]getline (myfile, PCourant->ancien);[/B]
        }
        myfile.close();
    }

    else cout << "Unable to open file";
}


void main () {
 

    DossierProfesseur *dossierProfesseur;
    
    dossierProfesseur= new DossierProfesseur;
}

[John A]

You're trying to use getline() to read a string into an integer. It just ain't going to work. Read it into a string, and take it out with atoi() or use a stringstream.

std::string line;
std::stringstream lineStream;
getline(myFile, line);
lineStream << line;
lineStream >> PCourant->ancien;

Your code is driving me crazy! If you're going to use char arrays, you might as well be using C. And no, I know it's not your fault, so I blame your teacher for that.

[earlyriser]

Thanks joeprogrammer!
I will try that and give the feedback tomorrow, (I have been doing this homework all the day).
I didn't know that C is better for char arrays than C++.

[John A]

Thanks joeprogrammer!
I will try that and give the feedback tomorrow, (I have been doing this homework all the day).
I didn't know that C is better for char arrays than C++.

It's better because C functions were designed for char arrays. A lot of C++ functions were designed for std::string. You could always use the old C functions to manipulate the mix of C and C++ strings, but you usually shouldn't mix C and C++ functions unless it's absolutely necessary.

[Ancient Dragon]

I didn't know that C is better for char arrays than C++.

That's not true, character arrays can be handled equally by either language.

[John A]

That's not true, character arrays can be handled equally by either language.

Well, yes.

My point was that it's like this. Say you want to input a string using getline():

#include <string>
std::string line;
getline(cin, line);

Now that's all fine and dandy. But let's say you want to copy this to a char string:

#include <cstring>
char name[20];
strcpy(name, line.c_str()); // should error check this

But now you've just had to resort to C to process the string. Sure there are probably workarounds, but generally it's a much, much better idea to use std::strings (although not in all cases; for example some constructors such as std::stream require C-strings).

[WaltP]

If you're going to use char arrays, you might as well be using C. And no, I know it's not your fault, so I blame your teacher for that.

Seems to me the concept here is to learn to process c-strings, because they are necessary in C++.

earlyriser, stop using any and all functions that have a definition of string and use only char* functions. Help us save Joe's sanity. :)

[Ancient Dragon]

Seems to me the concept here is to learn to process c-strings, because they are necessary in C++.

earlyriser, stop using any and all functions that have a definition of string and use only char* functions. Help us save Joe's sanity. :)

He's already said his instructor says he must use char arrays. He should be writing a pure C program, not a C++ program basterdised with C. If that is what his instructor is teaching then the instructor needs to take a few more programming courses.

[earlyriser]

Hi. In fact the Homework is in C++.
Maybe the error is mine (I dont want to embarras my teacher's reputation).
The structs provided in the HW are below, with the chnages I did because I wnated to reduce the difficulty and have less pointers. Maybe that was the thing that brought the insanity.

struct  Professeur{
   char*   nom; // [B]changed to char nom[20][/B]
   char*   prenom;
   Cours*  listecours;
   Etudiants*  listetudiants;
   Professeur* suivant;
};
 
struct Cours {
   char     * sigle; // [B]changed to char sigle[10][/B]
   Cours  * suivant;
};
 
struct  Etudiant {
   char*     nom; //changed to char nom[20]
   Etudiant* apres;
};

By the way the point of the HW are classes and not the string manipulation

[iamthwee]

>If that is what his instructor is teaching then the instructor needs to take a few more programming courses.

Not really, like Mr disney said it's nice to know about char[] style strings. In fact you'll find a lot of institutions teach kids c++ this way.

Will they come out knowing true c++? Nup, but there ya go. :lol:

[Ancient Dragon]

>>By the way the point of the HW are classes

Well, so much for your teacher's reputation, it just went into the gutter :eek: But we should be discussing your HW, not your teacher.

Had you left the pointers there it would have made your program just a small tad more complex but a whole lot safer because you could have allocated memory of the correct size for the strings, something like this:

PCourant->nom = new char[line.size()+1];
 strcpy (PCourant->nom, line.c_str());

Don't forget to delete[] to memory when done with it.

[earlyriser]

Thanks for all your tips. I will close this thread, because I have started another one with a less specific topic:
http://www.daniweb.com/techtalkforums/post317987.html#post317987

[Lazaro Claiborn]

After reading a line in a text file I need to read another line which contains an INT. I thought that the bold line in this code will be ok to retreive the data, but it doesnt.

#include <iostream>
#include <fstream>
#include <string>
#include <cstdlib>
using namespace std;
 
struct Professeur
{
    char nom[20];
    int ancien;
};
 
 
class DossierProfesseur
{
 
 
    public:
        Professeur *tete;
        DossierProfesseur();
        ~DossierProfesseur();
};
 
 
DossierProfesseur::DossierProfesseur()
{
    Professeur *PCourant, *PTete=NULL;
    string line;
    ifstream myfile ("FP.txt");
    if (myfile.is_open())
    {
        while (! myfile.eof() )
        {
            getline (myfile,line);
            PCourant = new Professeur;
            strcpy (PCourant->nom, line.c_str());
            cout << PCourant->nom;
            [B]getline (myfile, PCourant->ancien);[/B]
        }
        myfile.close();
    }
 
    else cout << "Unable to open file";
}
 
 
void main () {
 
 
    DossierProfesseur *dossierProfesseur;
 
    dossierProfesseur= new DossierProfesseur;
}

Being the nerd I am, I'll give my opinion on this particular issue. If you're going to use a structure data type, I'd just use malloc instead of new to allocate memory on the heap:

PCourant = (struct Professeur*)malloc(sizeof(struct Professeur));

Or you can define a macro something similar to new such as:

#define Professor Professeur
#define new(x) (x*)malloc(sizeof(x))
 
PCourant = new(Professor);

And for the string variable line, you might as well use a character array of say 100 characters or etc:

#define MAX_LINE 100
char line[MAX_LINE];

This will save the trouble calling the c_str() string class function and perhaps a bit of proccessor time. So now you previous version would be ok. Last, don't forget to free the allocated memory via the "free" function.

This is just an alternative way, but I guess it boils down to the specifications of your assignment.

Good luck, Lamabot

[John A]

Being the nerd I am, I'll give my opinion on this particular issue. If you're going to use a structure data type, I'd just use malloc instead of new to allocate memory on the heap:

PCourant = (struct Professeur*)malloc(sizeof(struct Professeur));

Why would you want to do that? :rolleyes:

This is a C++ program. So then use C++ memory allocation methods. The OP's method of using new was fine. (Aside: 2 things the OP did do wrong; didn't use delete and there's a memory leak in the loop as I mentioned previously.)

And for the string variable line, you might as well use a character array of say 100 characters or etc:

#define MAX_LINE 100
char line[MAX_LINE];

Yeah, except that getline() requires a C++ string as an argument... that's why the OP used it in the first place.

And while I'm critcizing people's code (sorry, I can't help it!), trash that void main(). It's not standard, nor is it good practice.
http://users.aber.ac.uk/auj/voidmain.shtml

[Lazaro Claiborn]

This is a C++ program. So then use C++ memory allocation methods. The OP's method of using new was fine. (Aside: 2 things the OP did do wrong; didn't use delete and there's a memory leak in the loop as I mentioned previously.)...

Yeah, except that getline() requires a C++ string as an argument... that's why the OP used it in the first place.

If this were a C++ program why use strcpy instead of str::swap, swap or std::replace? And if you didn't notice, I mentioned it is an alternative method; I personally don't like to see a lot of commonly used C code in a C++ program, but my opinion alone. ;)

Oh.... if you're referring to the memory leak in the while loop, yes, I noticed you mentioned it. But you neglected to illustrate how to fix the problem - which I figured I do. I don't take credit for it however, just trying to help out.

I do agree that one should avoid using void main().

And while I'm critcizing people's code (sorry, I can't help it!)

Hardly criticizing but rather stating the obvious.

Good luck, LamaBot

[John A]

If this were a C++ program why use strcpy instead of str::swap, swap or std::replace?

Because the OP's instructor required him to use C strings. I already expressed my disgust to this requirement, but it's a requirement that has to be met.

And if you didn't notice, I mentioned it is an alternative method; I personally don't like to see a lot of commonly used C code in a C++ program, but my opinion alone. ;)

It still is a C++ program, and as long as it is, you should try to use C++ functions as much as possible. In some circumstances it's either impossible or impractical to do such a thing, but that did not seem to be the case here.

Oh.... if you're referring to the memory leak in the while loop, yes, I noticed you mentioned it. But you neglected to illustrate how to fix the problem

I explained it better in the previous thread, although it wasn't completely necessary, since it is expected that the OP has the knowledge of linked list traversal. If not, then there's some serious learning to be done.

[Lazaro Claiborn]

Because the OP's instructor required him to use C strings. I already expressed my disgust to this requirement, but it's a requirement that has to be met.

Ok that makes a load of sense. :confused:

It still is a C++ program, and as long as it is, you should try to use C++ functions as much as possible. In some circumstances it's either impossible or impractical to do such a thing, but that did not seem to be the case here.

I guess I don't like to use "new" unless declaring a new class instance. I rarely use it for a structure but I thought I'd remind if not inform the OP of the other way. Also, if that were true, then still, why not use the string::swap, swap or string::replace instead? Since it is not the case here, than you can't justify the usage of malloc instead of new.

I explained it better in the previous thread, although it wasn't completely necessary, since it is expected that the OP has the knowledge of linked list traversal. If not, then there's some serious learning to be done.

I admit it you surely did explain way better than I did. One of my down falls I think. Anyway, if the OP has the knowledge of linked lists then he should know how to properly use them, which is why I modified the while loop. If he doesn't, well, you and I know there is indeed serious learning to be done.

LamaBot

[John A]

Well, except that string::replace can only be operated on a C++ string. In other words, you can use it to replace contents of a C++ string from a C string, but not exactly the other way around...

And I guess std::swap works, except that it's a rather crude method of implementing it. What if you want to make 2 identitcal strings, copying the contents of the original into a second one? Well, with swap it does not do that; it swaps them, like its name says. Using swap might work in some cases, like this example, where line is just thrown away. However, it's not the most elegant way of doing it, and I believe that strcpy() still does a better job.

Regardless of the copying methods, there shouldn't even be a need to copy very many C strings. So my feelings extend towards the OP. ;)

[Lazaro Claiborn]

You're right swap and replace are not suitable for this OPs code, that wasn't my point though. If I really would of suggested something it'd probably would of been the following:

eos = line.copy(PCourant->nom, 20, 0);
PCourant->nom[eos] = '\0';

Good luck OP, LamaBot

[iamthwee]

Oh.... if you're referring to the memory leak in the while loop, yes, I noticed you mentioned it. But you neglected to illustrate how to fix the problem - which I figured I do. I don't take credit for it however, just trying to help out.

You just don't use malloc with c++ period. Unless you want undefined behaviour.
http://www.informit.com/guides/content.asp?g=cplusplus&seqNum=33&rl=1

The other points, like using char[] in c++. Well that's just preference, c++ is after all a multiparadigm programming language.

You're right swap and replace are not suitable for this OPs code, that wasn't my point though. If I really would have suggested something it'd probably would have been the following:

And while I'm crit-e-sizing ppls grammer. (sorry, I can't help it!), Tee he he.

[~s.o.s~]

You just don't use malloc with c++ period. Unless you want undefined behaviour.

That would be only if you use them to allocate memory for non-POD (plain old data). For datatypes like integers, floats etc. you can still use malloc though is much frowned upon in the C++ community.