0

This is my first time using c++ and I am having trouble manipulating a set. The function is supposed to iterate through the set, find the pair of Regions* that have the least distance between them, insert a new Region* and remove the two selected Regions* from the list. However, when I call s.insert() the set will be updated, but when the outer while loops begins again the set size is still the original. I commented out the s.erase() lines, but I was having the same issue. I have tried creating a new set and working off the one that is passed. Any help would be greatly appreciated. I have a feeling it has something to with pointers.

Region* reduce(set<Region*>& ls) {
    set<Region*> s = ls;
    int i = 0;
    std::set<Region*> remove;
    while (i < 5) { 
        double cur_smallest = 0.0;
        Region* smallest1 = new Region(0, 0, 0, 0);
        Region* smallest2 = new Region(0, 0, 0, 0);
        std::set<Region*>::iterator rIterator;
        std::set<Region*>::iterator regionsIterator = s.begin();
            while (regionsIterator != s.end()) {
                Region* current = *regionsIterator;
                if (s.size() == 1) {
                    return current;
                }
                rIterator = s.begin();
                while (rIterator != s.end()) {
                    Region* c = *rIterator;
                    double d = c->distance(*current);
                    if (d < cur_smallest) {
                        cur_smallest = d;
                        smallest1 = c;
                        smallest2 = current;
                    }
                    else if (d == cur_smallest) {
                        if ((getArea(c) + getArea(current)) <= (getArea(smallest1) + getArea(smallest2))) {
                            smallest1 = c;
                            smallest2 = current;
                        }
                    }
                    rIterator++;
                }
                regionsIterator++;
            }
            Region tmp = *(smallest1,smallest2);
            s.insert(&tmp);
            //s.erase(smallest1);
            //s.erase(smallest2);
            cout << s.size();
            i++;
        }
sengineer23
  • 33
  • 4
  • 1
    You insert an address of the local variable `tmp` into the set. At the end of each iteration of the outermost loop, `tmp` is destroyed, and `s` ends up holding a dangling pointer. Any attempt to use that pointer exhibits undefined behavior. Also, chances are high that `tmp` ends up allocated at the same address on every iteration, so you end up inserting the same pointer; of course, only one copy is inserted. – Igor Tandetnik Apr 18 '21 at 22:35
  • @IgorTandetnik so how would I allocate memory for tmp? do I need to malloc space for it? – sengineer23 Apr 18 '21 at 22:38
  • Did you mean to make a copy of the set here `set s = ls;`? I assume you probably didn't, otherwise passing in a non-const reference is pointless. – Retired Ninja Apr 18 '21 at 22:39
  • Well, how did you populate `ls` in the first place? – Igor Tandetnik Apr 18 '21 at 22:40
  • 1
    `Region tmp = *(smallest1,smallest2);` -- What is this line supposed to do? Also, why does the set have raw pointers to `Region`? Why not `std::set`? – PaulMcKenzie Apr 18 '21 at 22:41
  • @PaulMcKenzie the , operator is overloaded to join the two regions and return a new Region* – sengineer23 Apr 18 '21 at 22:42
  • So you're new to C++, yet want to use some of the most obscure parts of it that only an advanced C++ programmer would even attempt to use.. – PaulMcKenzie Apr 18 '21 at 22:43
  • @IgorTandetnik the passed in set is generated manually for now – sengineer23 Apr 18 '21 at 22:43
  • What do you mean "manually"? There must be code somewhere that generates it. – Igor Tandetnik Apr 18 '21 at 22:44
  • Based on this code I'm going to assume the overloaded comma operator is also broken. Consider putting together a [mcve]. Pro Tip: Never overload the comma operator. https://stackoverflow.com/questions/5602112/when-to-overload-the-comma-operator – Retired Ninja Apr 18 '21 at 22:44
  • @IgorTandetnik int size; set regions; fstream fin; fin.open(filename, fstream::in); // READ THE REGIONS INTO A SET fin >> size; for (int i = 0; i < size; i++) { int x, y, w, h; fin >> x >> y >> w >> h; regions.insert(new Region(x, y, w, h)); } // GENERATE THE HIT-TESTING TREE Region* root = reduce(regions); – sengineer23 Apr 18 '21 at 22:46
  • In the code in your most recent comment, you show how to allocate memory for `Region`. So you must know how. – Igor Tandetnik Apr 18 '21 at 22:48
  • `Region* smallest1 = new Region(0, 0, 0, 0);Region* smallest2 = new Region(0, 0, 0, 0);` -- This is a memory leak if the `s.size()` is 1. – PaulMcKenzie Apr 18 '21 at 22:48
  • @RetiredNinja Ninja Region* Region::operator,(Region& other) { int x1; int y1; int w1; int h1; if (this->x < other.x) { x1 = this->x; } else { x1 = other.x; } if (this->y < other.y) { y1 = this->y; } else { y1 = other.y; } int dif1 = this->x + this->width; int dif2 = other.x + other.width; if (dif1 > dif2) { w1 = dif1 - x1; } else { w1 = dif2 - x1; } dif1 = this->y + this->height; dif2 = other.y + other.height; if (dif1 > dif2) { h1 = dif1; } else { h1 = dif2; } Region res = Region(x1, y1, w1, h1, this, &other); return &res; } – sengineer23 Apr 18 '21 at 22:48
  • @sengineer23 Update the question itself with the code. You see the code is unreadable within the comment. As it looks now, to be honest, this code looks like something pieced together without actually learning the C++ language properly. A relatively simple operation such as searching for two items in a set that match a criteria, and then removing them shouldn't be this complicated. – PaulMcKenzie Apr 18 '21 at 22:49
  • @PaulMcKenzie My apologies, it is not letting me edit the question. This is my latest attempt. Shouldn't this work? Region tmp = *(smallest1,smallest2); Region* tmp2 = &tmp; s.insert(tmp2); – sengineer23 Apr 18 '21 at 22:52
  • No, that code will not work. As stated earlier, you are storing the address of a temporary variable. These are the things you should have been aware of if you're going to write code this complex. C++ is one of the most difficult computer languages to learn, and to just learn it by "winging it" or in some sort of adhoc fashion isn't the way to learn it. Also, I highly suggest you write a `merge` function, and forget about overloading the comma operator. – PaulMcKenzie Apr 18 '21 at 22:54
  • @PaulMcKenzie I am very experience with Java, and fairly experienced with C. In C I would just malloc some space for a pointer and insert that. I am not sure how to do this in C++ – sengineer23 Apr 18 '21 at 22:56
  • *I am very experience with Java, and fairly experienced with C* -- None of those will prepare you for learning C++ properly,. Right now, it looks like you're using Java and C techniques in a C++ program. Forget those languages exist. For example, you use `new` as if you're coding in Java. – PaulMcKenzie Apr 18 '21 at 22:58
  • @IgorTandetnik Tandetnik I have found the solution, I feel pretty silly to be honest.... Region tmp = *(smallest1,smallest2); Region* tmp1 = new Region(tmp.x, tmp.y, tmp.width, tmp.height, tmp.r1, tmp.r2); – sengineer23 Apr 18 '21 at 22:59
  • You are leaking memory all over the place. Either use smart pointers such as `std::unique_ptr` or `std::shared_ptr`, or use just `set`. – PaulMcKenzie Apr 18 '21 at 23:00
  • 1
    As suspected, that comma operator is a disaster waiting to happen. Returning the address of a local variable. – Retired Ninja Apr 18 '21 at 23:05

1 Answers1

0
    Region tmp = *(smallest1,smallest2);
    Region* tmp1 = new Region(tmp.x, tmp.y, tmp.width, tmp.height, tmp.r1, tmp.r2);
    s.insert(tmp1);
sengineer23
  • 33
  • 4
  • 1
    This may help you limp along a little further but really isn't a solution. Your best bet is to just stop using pointers. – Retired Ninja Apr 18 '21 at 23:06