string assignment problem

[krebstar]

Hi guys,

I'm having a little problem setting a string value to another string..

I have the class:

class FILENAME
{
public:
	string GetFullPath() const;
	void SetFullPath(string& fname);
private:
	string fullpath;
};

The declaration for the SetFullPath Method is:

void FILENAME::SetFullPath(string& fname)
{
	fullpath = fname;
}

why is it that when I call the function with say,

else // it is a regular file, so push this onto our list
{
	fname = path + "\\" + ffd.cFileName;
	it->SetFullPath(fname);
	it++;
}

and run it, it fails with an access violation..? =/

What am I doing wrong here?

[chanda gul]

i think instead of directly assigning one string to another u should use strcpy function..try it...

[krebstar]

Already did so, by strcpy(fullpath,fname) but it doesn't work.. it tells me its a different type of string :(

I get compilation error:
error C2664: 'strcpy' : cannot convert parameter 1 from 'class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >' to 'char *'
No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called

[ArkM]

Where is it var declaration and initialization code?
This pointer variable must refer to FILENAME object. Obviously, it points to nowhere...

[krebstar]

variable and declaration?

Here is the code..

main :

int main()
{
	list<FILENAME> files;
	string path = "D:\\Source Code\\Homework";
	TraDir(path, files);

}

implementation file

#include "newtra.h"



void TraDir(string path, list<FILENAME>& files)
{
	list<FILENAME>::iterator it;
	it = files.begin();

..................................................

			else // it is a regular file, so push this onto our list
			{
				fname = path + "\\" + ffd.cFileName;
				it->SetFullPath(fname);
			}
		} while (FindNextFile(hFind,&ffd) !=0);

		FindClose(hFind);
	}
}

void FILENAME::SetFullPath(string& fname)
{
	fullpath = fname;
}

string FILENAME::GetFullPath() const
{
	return fullpath;
}

header file

#ifndef NEWTRA
#define NEWTRA


#pragma warning(disable: 4786)
#include <string>
#include <iostream>
#include <list>
#include <windows.h>

using namespace std;

class FILENAME
{
public:
	string GetFullPath() const;
	void SetFullPath(string& fname);
private:
	string fullpath;
};

void TraDir(string path, list<FILENAME>& files);
	


#endif

[ArkM]

It's not interesting code fragments.

Although class FILENAME is a clumsy and useless "wrapper" for file name string (full uncontrolled access to one and only one only private variable;)), it works as expected. So your problem, obviously, does not bear a relation to string assignment.

Let me ask again: where is the last (before fated using) assignment to it iterator? Have you it != files.end() test? Are you sure that files list is not empty?

For example, you can't dereference it == files.end(), you can't ++it if it == files.end() - and so on. I think, bad it value is the only reason of all these troubles.

Add tracing code, check iterator position as you wish...

[mitrmkar]

Your code probably should be doing what the comment there already says (without any iterator(s))...

else // it is a regular file, so push this onto our list
{
	fname = path + "\\" + ffd.cFileName;

        // assuming you have a FILENAME object ...
	aFILENAME.SetFullPath(fname);

        // now at to the end of the list ...
	files.push_back(aFILENAME);
}

[krebstar]

It's not interesting code fragments.

Although class FILENAME is a clumsy and useless "wrapper" for file name string (full uncontrolled access to one and only one only private variable;)), it works as expected. So your problem, obviously, does not bear a relation to string assignment.

Let me ask again: where is the last (before fated using) assignment to it iterator? Have you it != files.end() test? Are you sure that files list is not empty?

For example, you can't dereference it == files.end(), you can't ++it if it == files.end() - and so on. I think, bad it value is the only reason of all these troubles.

Add tracing code, check iterator position as you wish...

'

I couldn't think of a better classname for it so I used FILENAME hehe.. Failure of the imagination I guess.. XD

I think the iterator works as it should, I ran the debugger on it, and the access violation occurs when the SetFullPath method is invoked, right on the line where i assign fullpath=fname; .. but if I run cout << fname right before that statement I am getting the right results..

I don't understand exactly how I'm supposed to check iterator position, as I have only declared the iterator with the statement list<FILENAME>::iterator it; and then initialized it to point to the list of "files" object which is of type FILENAME, like so it = files.begin(); ..

to mitrmkar: I tried your suggestion, but its confusing.. what is "aFILENAME" supposed to be? an instance of the class FILENAME? I already have that, its called "files"..

Thanks guys for taking time to read this, I hope I figure it out soon..

[krebstar]

Could it be that I don't have a FILENAME object?

Maybe I should: FILENAME* filePtr = new FILENAME; and then filePtr->SetFullPath(fname); and then files.push_back(*filePtr) Does that sound right?

Hmm, i'm gonna go check it out..

edit: it compiles properly and doesnt have errors when I run it! now to check if the list items are being pushed back properly! update in a bit!

[CoolGamer48]

i think instead of directly assigning one string to another u should use strcpy function..try it...

Kind of off topic, but strcpy() is meant for use with C-Strings (i.e., char*). When using std::strings (which you should :) )you use the (overloaded? ) assignment operator.

[krebstar]

Kind of off topic, but strcpy() is meant for use with C-Strings (i.e., char*). When using std::strings (which you should :) )you use the (overloaded? ) assignment operator.

Yes I think that sounds about right :)

[krebstar]

Woo I got it guys!

Turns out I forgot to put the "fname" element into a FILENAME object first before pusing it onto the list! My function call was trying to assign fname to a nonexistent "fullname" field :)

THanks, couldn't have done this without you guys! :)

[mitrmkar]

Maybe I should: FILENAME* filePtr = new FILENAME; and then filePtr->SetFullPath(fname); and then files.push_back(*filePtr) Does that sound right?

That would leak memory because push_back() invokes the copy constructor of FILENAME i.e. you would not be storing the new'ed object anywhere, instead use simply ...

FILENAME file;
file.SetFullPath(fname);
files.push_back(file);

[krebstar]

That would leak memory because push_back() invokes the copy constructor of FILENAME i.e. you would not be storing the new'ed object anywhere, instead use simply ...

FILENAME file;
file.SetFullPath(fname);
files.push_back(file);

would it still cause memory leak if i did this?

else // it is a regular file, so push this onto our list
			{
				fname = path + "\\" + ffd.cFileName; // this is the full path name that we want to push into our list
				
				FILENAME* filePtr = new FILENAME; // create a FILENAME pointer to a FILENAME object on the heap
				
				filePtr->SetFullPath(fname); // pass the "fname" into the FILENAME object on the heap
				
				files.push_back(*filePtr); // push the FILENAME object on the heap into the "files" list

				delete filePtr;
			}

At any rate, would it simply be better to point the pointer to a static object like "file" in your example, rather than allocating one on the heap, in terms of performance? Thanks..

[mitrmkar]

would it still cause memory leak if i did this?

else // it is a regular file, so push this onto our list
			{
				fname = path + "\\" + ffd.cFileName; // this is the full path name that we want to push into our list
				
				FILENAME* filePtr = new FILENAME; // create a FILENAME pointer to a FILENAME object on the heap
				
				filePtr->SetFullPath(fname); // pass the "fname" into the FILENAME object on the heap
				
				files.push_back(*filePtr); // push the FILENAME object on the heap into the "files" list

				delete filePtr;
			}

At any rate, would it simply be better to point the pointer to a static object like "file" in your example, rather than allocating one on the heap, in terms of performance? Thanks..

delete filePtr; would prevent any leaks.

files.push_back(*filePtr); // push the FILENAME object on the heap into the "files" list

That does NOT push the object on the heap into the list, instead it invokes the copy constructor of FILENAME constructing the object that ends up into the list.
If you write yourself a copy constructor ...

FILENAME(const FILENAME & o)
	{
		cout << "FILENAME / copy constructor\n";
		name = o.name;
	}

that will display the text each time you call files.push_back(...) .

If your list would be declared as list <FILENAME *> files; then you'd need to be new'ing the objects you store in the list. But since you are storing FILENAME objects, no pointers/explicit heap allocations whatsoever need to be involved.

[krebstar]

delete filePtr; would prevent any leaks.

That does NOT push the object on the heap into the list, instead it invokes the copy constructor of FILENAME constructing the object that ends up into the list.
If you write yourself a copy constructor ...

FILENAME(const FILENAME & o)
	{
		cout << "FILENAME / copy constructor\n";
		name = o.name;
	}

that will display the text each time you call files.push_back(...) .

If your list would be declared as list <FILENAME *> files; then you'd need to be new'ing the objects you store in the list. But since you are storing FILENAME objects, no pointers/explicit heap allocations whatsoever need to be involved.

whoops, I meant to say,

push the FILENAME object (that is on the heap) into the list called "files"..

is that right?

I'm confused..

I did not declare my list as: list <FILENAME *> files; btw, I changed it to vectors now, I think it might be a little better..

Could you clarify what you meant with the last paragraph?

isn't this a pointer/heap allocation:? FILENAME* filePtr = new FILENAME;

[mitrmkar]

whoops, I meant to say,

push the FILENAME object (that is on the heap) into the list called "files"..

is that right?

No, the point is that the specific object that you have on the heap, is NOT stored into the list. Instead, the derefenced pointer which you supply to push_back(..) is only used to construct another object (via copy constructor) that IS stored into the list. The initial object on the heap will happily remain there unless you issue delete on it.

I did not declare my list as: list <FILENAME *> files;

I know, I wrote "If your list would be declared as list <FILENAME *> files;", maybe you missed it.

Could you clarify what you meant with the last paragraph?

isn't this a pointer/heap allocation:? FILENAME* filePtr = new FILENAME;

Yes it is.

Once again ... no pointers/explicit heap allocations whatsoever need to be involved. I.e. you can write:

vector < FILENAME > files;
string fname;

for( ... some loop condition here ... )
{
    fname =  ... some code here that builds the file path ...

    FILENAME file;
    file.SetFullPath(fname);
    // invoke copy constructor and store a new object (a copy of the 'file' object) 
    files.push_back(file);
}

[krebstar]

Oh hehe okay, I get it now :P Pardon me if I'm bein slow, still not quite comfortable with everything yet.. :)

Thanks for explaining! :)