Why is the pointer not pointing?

[oghmaosiris]

So, I'm trying to open (or create for the output files) 4 files to use in this payroll program I was assigned.

The thing will compile, but when I run, filenamePtr doesn't point to the character address that is returned by BuildFileName...

anyone have any tips or see what I'm not?

ErrorStatus OpenFiles( ifstream& masterIn, ifstream& timeClockIn, ofstream& masterOut, ofstream& detailOut )
{
      char* filenamePtr;
 	ErrorStatus error = NONE;
   int weekNumber = GetWeekNumber( );

   // repeat this for each of the files to be opened
   filenamePtr = BuildFilename( "Master", weekNumber, ".txt" );
   cout << filenamePtr << endl;//test return
   masterIn.open( filenamePtr );
   if ( !masterIn ) {
      error = error | MASTER_IN;
   }//endif
      filenamePtr = BuildFilename( "TimeClock", weekNumber, ".txt" );
   timeClockIn.open( filenamePtr );
   if ( !timeClockIn ) {
      error = error | TIMECLOCK_IN;
   }//endif
      filenamePtr = BuildFilename( "MasterOutput", weekNumber, ".txt" );
   masterOut.open( filenamePtr );
   if ( !masterOut ) {
      error = error | MASTER_OUT;
   }//endif
      filenamePtr = BuildFilename( "Detail", weekNumber, ".txt" );
   detailOut.open( filenamePtr );
   if ( !detailOut ) {
      error = error | DETAIL_OUT;
   }//endif

   return error;
}//endfn OpenFiles


char* BuildFilename( char* primary, int weekNumber, char* extention)
{
   char fileName[64] = "";
   char numberToChar[3];
   itoa (weekNumber,numberToChar, 10);

   //cout << numberToChar << endl;

   strcat(fileName, primary);
   //cout << fileName << endl;
   strcat(fileName, numberToChar);
   //cout << fileName << endl;
   strcat(fileName, extention);
   cout << fileName << endl;//test
   char* returnName = fileName;

   return returnName;
}//endfn BuildFilename 

[Lv0f92yv]

I'm not positive, though I compiled a short bit that sort of emulates what it looks like you're trying to do:

#include <iostream>
using namespace std;
char* getptr();

void main()
{
	char* ptr;
	ptr = getptr();
	cout << ptr;
	getchar();
	getchar();
}

char* getptr()
{
	char* test = "test";
	char* ret = test;
	return ret;
}

This seems to work, maybe there are some ideas in this code you can use.

[oghmaosiris]

That looks like the same thing that I did. Logically, I don't see the differences.

[Lv0f92yv]

I have been playing around with this for a bit with no luck... I'm sure there's a way to do it. Alternatively, you can try passing in strings to the function and using strncat to make the final string, which you can return. To use it with a filestream, you can use:

fstream file;
file.open( filename.c_str(), ios::out ... )

with .c_str().

Edit:

With something like this:

#include <iostream>
#include <string>
using namespace std;
string getptr( const char*, int, const char* );

void main()
{
	string ptr;
	ptr = getptr( "prim", 5, ".txt" );

	cout << ptr;
	getchar();
	getchar();
}

string getptr( const char* primary, int weekNumber, const char* extention)
{
   string fileName;
   char numberToChar[10];
   itoa (weekNumber,numberToChar, 10);
   fileName += primary;
   fileName += numberToChar;
   fileName += extention;
   return fileName;
}//endfn BuildFilename  

you could use file.open( ptr.c_str(), ...)

etc... Probably not the best way to do it, but it should work.

[tummychow]

You're using the pointer as a char* (equivalent to char[]), right? To store some kind of string? Then converting to std::string will be much easier, because string can be treated as a type, where as char[] has to be handled as an array/pointer. If you make that switch, you should be able to eliminate a lot of your dereferencing/addressing problems.

[guestgulkan]

In the original code - he is returing a pointer to a local variable -

char* BuildFilename( char* primary, int weekNumber, char* extention)
{
   char fileName[64] = ""; //local array variable 
    //Other code
   
    char* returnName = fileName;
   return returnName; //<<== Returing pointer to local variable - Not good
}

**NOTE**
To Desh

char* getptr()
{
	char* test = "test";
	char* ret = test;
	return ret;
}

This is not quite the same as the original poster's problem, bcause in this case the string data
is not local to the function.

[oghmaosiris]

Why can't I return a char address to the function that called it?

Its being stored into another char address variable. SHould I just return fileName? Seeing as that's a char address, too.

And to tummychow, I'm not allowed to use strings in my assignments because we can do the same thing with char arrays so there's no overhead.

[guestgulkan]

oghmaosiris wrote:

Why can't I return a char address to the function that called it?

It is not the return of a char* pointer that is the problem - it is where the data that it
is pointing to is located.
It is pointing to the fileName array.
This array is local to the BuildFilename function - when a function completes, all the
space used by it's local variables is released and is up for grabs to be used by the next function called.

You do not notice it, and the compiler does not complain, because you copy the address of the
array to another variable (returnName).

But as returnName now points to the array ( it is now the same as fileName) - you can rewite
your function like this

char* BuildFilename( char* primary, int weekNumber, char* extention)
{
   char fileName[64] = "";
   char numberToChar[3];
   itoa (weekNumber,numberToChar, 10);

   //cout << numberToChar << endl;

   strcat(fileName, primary);
   //cout << fileName << endl;
   strcat(fileName, numberToChar);
   //cout << fileName << endl;
   strcat(fileName, extention);
   cout << fileName << endl;//test
  // char* returnName = fileName; //<<========forget about this

   return fileName; //<<==========Notice this 
}//endfn BuildFilename  

Now the compiler has a clear idea of what you are doing - and you will get the following warning:
(or similar)
warning : returning address of local variable or temporary

warning : address of local variable 'fileName' returned

[oghmaosiris]

Ok. When I return just fileName, there's random garbage in front of the array.

edit: Your explination made sense, btw. It helped me understand what was happening, lol... Just thought I'd throw that out there.

[Lv0f92yv]

Very good explanation guestgulkan. I always tend to forget arrays are the same as pointers.

[oghmaosiris]

GOT IT! Finally.. I used dynamic arrays ;)

[oghmaosiris]

Here's what I ended up doing. Is there a more efficient way of going about this? If so, could someone explain how its more efficient? Purely for experience's sake.

ErrorStatus OpenFiles( ifstream& masterIn, ifstream& timeClockIn, ofstream& masterOut, ofstream& detailOut )
{
      char* fileNamePtr = NULL;
      fileNamePtr = new char[64];
      for (int i=0; i<64; i++) {
         fileNamePtr[i] = NULL;    // Initialize all elements to zero.
      }
 	ErrorStatus error = NONE;
   int weekNumber = GetWeekNumber( );

   // repeat this for each of the files to be opened
   fileNamePtr = BuildFilename( "Master", weekNumber, ".txt", fileNamePtr );
   cout << fileNamePtr << endl;
   masterIn.open( fileNamePtr );
   if ( !masterIn ) {
      error = error | MASTER_IN;
   }//endif
   delete [] fileNamePtr;
   fileNamePtr = new char[64];
      for (int i=0; i<64; i++) {
         fileNamePtr[i] = NULL;    // Initialize all elements to zero.
      }
      fileNamePtr = BuildFilename( "TimeClock", weekNumber, ".txt", fileNamePtr );
   timeClockIn.open( fileNamePtr );
   if ( !timeClockIn ) {
      error = error | TIMECLOCK_IN;
   }//endif
   delete [] fileNamePtr;
   fileNamePtr = new char[64];
      for (int i=0; i<64; i++) {
         fileNamePtr[i] = NULL;    // Initialize all elements to zero.
      }
      fileNamePtr = BuildFilename( "MasterOutput", weekNumber, ".txt", fileNamePtr );
   masterOut.open( fileNamePtr );
   if ( !masterOut ) {
      error = error | MASTER_OUT;
   }//endif
   delete [] fileNamePtr;
   fileNamePtr = new char[64];
      for (int i=0; i<64; i++) {
         fileNamePtr[i] = NULL;    // Initialize all elements to zero.
      }
      fileNamePtr = BuildFilename( "Detail", weekNumber, ".txt", fileNamePtr );
   detailOut.open( fileNamePtr );
   if ( !detailOut ) {
      error = error | DETAIL_OUT;
   }//endif
   delete [] fileNamePtr;

   return error;
}//endfn OpenFiles 

[helios]

The only reason I'm posting such a complete sample is to demonstrate a trick to avoid repeating code:

std::string suffix; //#include <string>
{
    std::stringstream stream; //#include <sstream>
    stream <<weekNumber;
    suffix=stream.str();
    suffix.append(".txt");
}

const char *prefixes[]={
    "Master",
    "TimeClock",
    "MasterOutput",
    "Detail",
    0
};
ErrorStatus error=NONE,
    errors[]={
        MASTER_IN,
        TIMECLOCK_IN,
        MASTER_OUT,
        DETAIL_OUT
    };
std::fstream *files[]={
    &masterIn,
    &timeClockIn,
    &masterOut,
    &detailOut
};

for (int a=0;prefixes[a];a++){
    files[a]->open((prefixes[a]+suffix).c_str());
    if (!*files[a])
        error|=errors[a];
}

[oghmaosiris]

I don't really understand what you're trying to tell me with that code snippet.

[Disch]

helios is using lookup tables to have the information for the different files.

Instead of copy/pasting the same code multiple times, and making minor changes each time, he just puts the whole thing in a loop, and uses the loop counter to index all of the lookup tables.

This is benefitial because it removes duplicate code. Duplicate code is problematic because:

1) it's messy. Compare the code in your post to helios'. Isn't helios' much easier to read?)

2) It's harder to maintain. What happens if you want to change the logic a little bit in the future. You'd have to remember to make the change multiple times in your code (once for each file), whereas with helios' approach, all of the code is in one spot, so he would only have to make the change once.

3) It's more error prone. It's very easy to make mistakes when copy/pasting code multiple times. Something as simple as forgetting to change MASTER_IN to MASTER_OUT after a paste could break your code and might be very hard to track down when debugging.


Helios also is using std::string instead of allocating/destroying a buffer multiple times, but that's another issue.

[oghmaosiris]

Ok, that makes sense. I still can't use strings, tho.

[Disch]

Right.. I missed that part...

And to tummychow, I'm not allowed to use strings in my assignments because we can do the same thing with char arrays so there's no overhead.

That's kind of a bogus reason though. std::string has little/no overhead when compared to char arrays. Sometimes, it's even faster (and it's always safer).

Anyway you can still use helios' approach with the lookup tables and just get rid of the string/stringstream usage.

[oghmaosiris]

Hey, I would use strings if I could. I find it much faster, but I get why I shouldn't lol... Learning the fundamentals is more important than shortcuts/the-easy-way.

[Disch]

Learning the fundamentals is more important than shortcuts/the-easy-way

Then that's the reason your teacher should be giving you ;P

I have mixed feeling about it. I remember learning how to do long division in grade school and thinking it was absurd that we couldn't just use a calculator. I guess I must have learned some fundamental knowledge in the process, but I don't think I really did. To this day it still seems like a huge waste of time. I can count on one hand the number of times I've had to do long division by hand since learning it in school almost 2 decades ago.

Really... when are you in a situation where a calculator isn't available? Especially now with cell phones.

</thread derailment>

[oghmaosiris]

Well, I learned long division and now can do it in my head without a calculator, lol.

But I digress.

Thanks for the help everyone!