STL string comparison functor

Question

Hi all! I have the following functor:

class ComparatorClass {
  public:
    bool operator () (SimulatedDiskFile * file_1, SimulatedDiskFile * file_2) {
      string file_1_name = file_1->getFileName();
      string file_2_name = file_2->getFileName();

      cout << file_1_name << " and " << file_2_name << ": ";

      if (file_1_name < file_2_name) {
        cout << "true" << endl;
        return true;
      }
      else {
        cout << "false" << endl;
        return false;
      }
    }
};

It is supposed to be a strict weak ordering, and it's this long (could be one line only) for debug purposes.

I'm using this functor as a comparator functor for a stl::set. Problem being, it only inserts the first element. By adding console output to the comparator function, I learned that it's actually comparing the file name to itself every time.

Other relevant lines are:

typedef set<SimulatedDiskFile *, ComparatorClass> FileSet;

and

// (FileSet files_;) <- SimulatedDisk private class member
void SimulatedDisk::addFile(SimulatedDiskFile * file) {
  files_.insert(file);
  positions_calculated_ = false;
}

EDIT: the code that calls .addFile() is:

current_request = all_requests.begin();
while (current_request != all_requests.end()) {
  SimulatedDiskFile temp_file(current_request->getFileName(), current_request->getResponseSize());
  disk.addFile(&temp_file);
  current_request++;
}

Where all_requests is a list, and class Request is such that:

class Request {
  private:
    string file_name_;
    int response_code_;
    int response_size_;

  public:
    void setFileName(string file_name);
    string getFileName();
    void setResponseCode(int response_code);
    int getResponseCode();
    void setResponseSize(int response_size);
    int getResponseSize();
};

I wish I could offer my hypotesis as to what's going on, but I actually have no idea. Thanks in advance for any pointers.

Answer 1

There's nothing wrong with the code you've posted, functionally speaking. Here's a complete test program - I've only filled in the blanks, not changing your code at all.

#include <iostream>
#include <string>
#include <set>

using namespace std;

class SimulatedDiskFile
{
public:
 string getFileName() { return name; }

 SimulatedDiskFile(const string &n)
  : name(n) { }

 string name;
};

class ComparatorClass {
  public:
 bool operator () (SimulatedDiskFile * file_1, SimulatedDiskFile * file_2) {
   string file_1_name = file_1->getFileName();
   string file_2_name = file_2->getFileName();

   cout << file_1_name << " and " << file_2_name << ": ";

   if (file_1_name < file_2_name) {
  cout << "true" << endl;
  return true;
   }
   else {
  cout << "false" << endl;
  return false;
   }
 }
};

typedef set<SimulatedDiskFile *, ComparatorClass> FileSet;

int main()
{
 FileSet files;

 files.insert(new SimulatedDiskFile("a"));
 files.insert(new SimulatedDiskFile("z"));
 files.insert(new SimulatedDiskFile("m"));

 FileSet::iterator f;
 for (f = files.begin(); f != files.end(); f++)
  cout << (*f)->name << std::endl;

 return 0;
}

I get this output:

z and a: false
a and z: true
z and a: false
m and a: false
m and z: true
z and m: false
a and m: true
m and a: false
a
m
z

Note that the set ends up with all three things stored in it, and your comparison logging shows sensible behaviour.

Edit:

Your bug is in these line:

SimulatedDiskFile temp_file(current_request->getFileName(), current_request->getResponseSize());

disk.addFile(&temp_file);

You're taking the address of a local object. Each time around the loop that object is destroyed and the next object is allocated into exactly the same space. So only the final object still exists at the end of the loop and you've added multiple pointers to that same object. Outside the loop, all bets are off because now none of the objects exist.

Either allocate each SimulatedDiskFile with new (like in my test, but then you'll have to figure out when to delete them), or else don't use pointers at all (far easier if it fits the constraints of your problem).

Answer 2

And here is the problem:

SimulatedDiskFile temp_file(current_request->getFileName(),
                                   current_request->getResponseSize());
disk.addFile(&temp_file);

You are adding a pointer to a variable which is immediately destroyed. You need to dynamically create your SDF objects.

Answer 3

urrent_request = all_requests.begin();
while (current_request != all_requests.end()) {
  SimulatedDiskFile temp_file(...blah..blah..); ====> pointer to local variable is inserted
  disk.addFile(&temp_file);
  current_request++;

}

temp_file would go out of scope the moment next iteration in while loop. You need to change the insert code. Create SimulatedDiskFile objects on heap and push otherwise if the objects are smaller then store by value in set.

Answer 4

Agree with @Earwicker. All looks good. Have you had a look inside all_requests? Maybe all the filenames are the same in there and everything else is working fine? (just thinking out loud here)