Generic programming (not really Allegro related)

Generic programming (not really Allegro related)

TeamTerradactyl

Member #7,733

September 2006

In these attachments, I have a very simple program that simply takes command-line arguments and checks whether or not they are valid filenames.

It seems to work well, the debugger didn't report any SEGFAULTS, etc. The only problem I have is when I pass "invalid" filenames to the program: it "forgets" the next "valid" filename(s).

Example:

program.exe file1 file2 file3 file4

file[134] exist, while 'file2' does not.

My output SHOULD be:

Valid filenames found:
  file1
  file3
  file4

Instead, output is:

Valid filenames found:
  file1
  file4

Any idea why 'file3' is not showing up after 'file2' is NOT found?

Thanks for any ideas!

Indeterminatus

Member #737

November 2000

Filename &operator= is missing a return *this; statement.

Other than that, are you not allowed to use std::vector or std::list? Probably std::set would make even more sense, unless of course the order is important.

_______________________________
Indeterminatus. [Atomic Butcher]
si tacuisses, philosophus mansisses

TeamTerradactyl

Member #7,733

September 2006

I added <b>return this; to the &operator= as follows, but it did not seem to help:

Filename &Filename::operator=(const Filename &orig)
{
   numFiles = orig.numFiles;
   array = new string[numFiles];
   
   for (int curFilename = 0; curFilename < numFiles; curFilename++)
   {
      array[curFilename] = orig.array[curFilename];
   }
   
   return *this;
}

I'm thinking that the problem has to be lying within the 'filename.cpp' instead of the 'filename.h' file: The insertion and removal SEEMS to be working okay, but the "checking whether file is valid" loop would be where my guess is.

Instead of using

ifstream fin;
fin.open(...);
if (!fin || fin.fail())
{
   ...
}
fin.close();

I wouldn't mind using "exist()" (or is it "exists()" with an s?), but can't remember which library I need to include :-).

Indeterminatus: No real reason why I'm avoiding std::stuff other than the fact that I just want to make sure I'm understanding everything. (Better to reinvent the wheel so I can learn in the process, right?)

Thanks for your feedback; you were right about forgetting to return that value.

Does anyone have any ideas why this is skipping those files?

tobing

Member #5,213

November 2004

The problem is your usage of ifstream.

This works:

1Filename checkValidFilenames(int argc, char **argv)
2{
 Filename validFiles;
 ifstream fin;
5 
 for (int currentFile = 1; currentFile < argc; currentFile++)
 {
    fin.open(argv[currentFile]);
9 
    if (fin.fail())
    {
12//         fin.ignore();
       fin.clear();
    }
    else
    {
      validFiles.insert(argv[currentFile]);
      fin.close();
    }
 }
 
 return validFiles;
23}

Seems that the ifstream doesn't open a file correctly if closed after the failed open, or when ignore has been used.

Would be even better this way:

1Filename checkValidFilenames(int argc, char **argv)
2{
 Filename validFiles;
4 
 for (int currentFile = 1; currentFile < argc; currentFile++)
 {
    ifstream fin;
    fin.open(argv[currentFile]);
9 
    if (!fin.fail())
    {
      validFiles.insert(argv[currentFile]);
      fin.close();
    }
 }
 
 return validFiles;
18}

ifstream.open just uses fopen inside (implementation dependent of course), so it might be more efficient to use fopen directly.

Another remark: Your Filename leaks memory. Look at operator=(). And, all the reallocation of memory is REALLY inefficient. Not that it matters with that tiny application, but handling memory like this can slow down your app to a halt.

http://www.villages-and-cities.de

TeamTerradactyl

Member #7,733

September 2006

I think I found the problem with the missing "if file is valid":

1StringArray checkValidFilenames(const int &argc, char **argv)
2{
3   StringArray validFiles;
4   ifstream fin;
5 
6   for (int currentFile = 1; currentFile < argc; currentFile++)
7   {
8      fin.open(argv[currentFile]);
9 
10      if (fin.is_open())
11         validFiles.insert(argv[currentFile]);
12 
13      fin.close();
14   }
15 
16   return validFiles;
17}

Thanks for your feedback on that.

As far as the memory leak, tobing, I'll take a look at it. I'm not very good at assignment operators yet, so thanks for showing where the problem lies.

-TT