0

I can't use std::set_union because I'm not overloading the assignment operator correctly.

I'm using a std::set of my own struct, NodeChange_t, which itself contains another struct, point_t. Here are these guys' operator overloads: 

// ---- defs.h
struct point_t
{
    double x;
    double y;

    ...

    void operator=(const point_t &p)
    {
        x = p.x;
        y = p.y;
    }

    ...

};


struct NodeChange_t
{
    SNode node;
    point_t change;
    ListDigraph *graph;

    ...

    void operator=(const NodeChange_t &otherChange)
    {
        this->node = otherChange.node;
        this->change = otherChange.change;
        this->graph = otherChange.graph;
    }

    ...

};

// ---- _2DSurface.cpp

//Problematic code:
void _2DSurface::updateInnerSurfaceV2(_2DSurface &outerSurf, std::set<NodeChange_t> *nodeChanges)
{
    std::set<NodeChange_t> additions;

    ...

    // add stuff to additions
    std::set_union(additions.begin(), additions.end(), nodeChanges->begin(), nodeChanges->end(), nodeChanges->begin());

    ...

}

In this case, I want *nodeChanges to be overwritten. But the error I keep getting is:

src/_2DSurface.cpp:308:7: note: in instantiation of function template specialization
      'std::__1::set_union<std::__1::__tree_const_iterator<ct, std::__1::__tree_node<ct, void *> *, long>,
      std::__1::__tree_const_iterator<ct, std::__1::__tree_node<ct, void *> *, long>, std::__1::__tree_const_iterator<ct,
      std::__1::__tree_node<ct, void *> *, long> >' requested here
        std::set_union(nodeChanges->begin(), nodeChanges->end(), additions.begin(), additions.end(), nodeChanges.begin());
include/defs.hpp:258:7: note: candidate function not viable: 'this' argument has type 'const std::__1::__tree_const_iterator<ct,
      std::__1::__tree_node<ct, void *> *, long>::value_type' (aka 'const ct'), but method is not marked const
        void operator=(struct ct &otherChange)

How does it even make sense that an assignment operator would be marked const, if the whole point is to modify what's on the left hand side? I've been messing around with the const qualifier but can't seem to get anywhere. Any help is appreciated.

André Muricy
  • 69
  • 6
  • 1
    I'm pretty sure you cannot use set's begin as an output iterator because it will return const references to elements (you cannot, afaik, mutate elements in a set since that would break the internal ordering of the container). I believe you need a back inserter of some sort (as is shown [here](https://en.cppreference.com/w/cpp/algorithm/set_union)). finally i find it odd that your output range is one of the input ranges. Seems to me like that would break the algorithm. – Borgleader Jan 21 '19 at 02:17
  • Why are you mixing C style code in your C++ code? – Phil1970 Jan 21 '19 at 02:18
  • 1
    P.S: If your function parameters cannot be null, please pass them as references instead. – Borgleader Jan 21 '19 at 02:19
  • Why are you redefining `operator=` if you want to use default anyway? What is `struct ct`? Your code is unusual. – Phil1970 Jan 21 '19 at 02:19
  • 1
    There is no need for `typedef struct` in C++. Just use `struct`. – PaulMcKenzie Jan 21 '19 at 02:24
  • @Borgleader I tried outputting the result to another set's `.begin()` iterator, which also didn't work. But that shouldn't be what the issue is, at least according to the example on cplusplus.com: http://www.cplusplus.com/reference/algorithm/set_union/ – André Muricy Jan 21 '19 at 02:31
  • As for how terrible/unusual the code style is, I'm doing my best to refactor some old code I wrote for an undergraduate research project and turn it into an actually good open source program that scientists can use. The C-style just carried over from my job, really. EDIT: Made some changes so it's hopefully easier to read. @Phil1970 – André Muricy Jan 21 '19 at 02:33
  • 1
    @AndréMuricy Yes it's the problem. Look at the output container in the example you linked, it's a vector not a set (and a vector that's been resized in advance no less). set & vector have different semantics, vector doesnt have an internal ordering to maintain, set does. – Borgleader Jan 21 '19 at 02:41
  • 2
    Why aren't both your `operator=` returning references to the current object, just as any other usual `operator=` would do? Second, you have a pointer member, so are you following the [rule of 3](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three)? Third, you have `...` in your post. If you have more members than those 3 you have in your assignment operator, you are creating copies that are partial copies, not full copies. Doing things this way leads to bugs that are hard to find. Last, what was wrong with the default assignment operator? Why are you overloading `=`? – PaulMcKenzie Jan 21 '19 at 02:45
  • @AndréMuricy You updated the code and fix some naming issue but error message still use `struct ct`. By the way, why not take 5 minutes to make a MVCE (https://stackoverflow.com/help/mcve). – Phil1970 Jan 21 '19 at 03:11

1 Answers1

2

How does it even make sense that an assignment operator would be marked const, if the whole point is to modify what's on the left hand side?

The assignment operator is not marked const. In fact, the error message says as much; it is one of the triggers for the error. Take another look at the relevant parts of the error message with some key emphasis:

candidate function not viable: 'this' argument has type [snipped] (aka 'const ct'), but method is not marked const

The other trigger for the error is that the object receiving the assignment is marked const. If you look around the types mentioned in the error message, you might notice "const_iterator". This is your clue! Somewhere a constant iterator is being de-referenced and a value assigned to the result. All iterators involved are set iterators, so let's take a look at documentation for set. A set's iterator type is a constant iterator; you cannot write to it. (For a set, the iterator and const_iterator types are usually aliases for the same type. This redundancy allows an interface that is consistent with other containers.)

The set_union algorithm requires an output iterator for the destination. A set does not have an output iterator. So even though the word "set" appears in "set_union", set_union cannot directly output a set.

Also concerning is another detail from the set_union documentation: "The resulting range cannot overlap with either of the input ranges." You cannot accomplish what you want (replace one of the inputs with the union) in one step with set_union. If that is the tool you want to use, you'll need to output the union to an auxiliary container, then update nodeChanges in a separate step. Hoewever, it would probably be simpler to use set::insert (variation 5):

nodeChanges->insert(additions.begin(), additions.end());
JaMiT
  • 14,422
  • 4
  • 15
  • 31
  • Ahh, I see. Thanks so much! I should indeed have gone into set's documentation, for whatever reason I took for granted the fact that it would have similar semantics to vector when it comes to the default iterator getters. But this was a very helpful answer :) – André Muricy Jan 21 '19 at 13:04