Cannot assign a Data from a DataConstRef

[LourensVeen]

Assigning to an int & from an int const & works fine, so this should also be possible, shouldn't it?

[LourensVeen]

Fixing this should allow making libmuscle/cpp/src/libmuscle/snapshot.cpp a bit prettier.

[LourensVeen]

Wait, an int is a basic type, and the assignment above copies it. But Data/DataConstRef can also contain strings, lists, dicts, byte arrays, and grids, which aren't basic types and get linked, not copied. So allowing assignment of those from a DataConstRef would remove the constness, because you could subsequently modify the object through the Data object.

C++ doesn't have that issue because everything is by value and so copied, and Python and Java don't have that issue because they don't have const.

The original issue was that we couldn't assign a DCR from a DCR, which was needed to unpack a tuple using std::tie. This makes sense, because DCR::operator=(DCR const &) is deleted, it's a const ref, so you cannot assign through it. We do have DCR::reseat(DCR const &) which does what we want, but of course std::tie doesn't know about that.

Then we had another issue (in snapshot.cpp:103), where we had a Data containing a dictionary, and we want to set a value to a DCR for a given key. So you want to be able to do dict["key"] = dcr; and that doesn't work.

So what are the options?

1. Leave it as is
2. Allow the assignment, casting away constness
3. Allow the assignment, and always copy the value

Option 2. is not acceptable, 1. leaves a few places internally where the code isn't as pretty as it could be, and 3. is potentially inefficient and confusing. It seems like 1. may be the way to go actually.

Maybe the problem is that I should have done a DataConstPtr instead of a DataConstRef? Then the assignment using std::tie would copy the pointer and all would be well. That's water under the bridge though. And a reference has its advantages too.

[maarten-ic]

For complex types, it should still be possible to have two references to the same const object? I think the following should be allowed:

DataConstRef dcr = DataConstRef::list(1, 2, 3);

// Should be allowed:
DataConstRef just_another_reference = dcr;

// Should not be allowed:
Data casting_away_constness = dcr;

But by allowing DCR::operator=(DCR const &), you allow the first option. Since Data is a derived type of DCR I would not expect that there is a default Data::operator=(...) that allows passing a DCR in, so still the second option is prohibited?

[maarten-ic]

Some more thoughts:

// These do not exist, but should? Allow passing basic types, Data and DCR as args
DataConstRef dcr1 = DataConstRef::list(Args...);
DataConstRef dcr2 = DataConstRef::dict(Args...);

// These exist and currently allow for upcasting of DCR to Data!
// -> The args should only accept basic types and Data, not DCR
Data data1 = Data::list(Args... args);
Data data2 = Data::dict(Args... args);

[LourensVeen]

The problem is, if you assign to a reference, what do you assign? Does it modify the referenced object, or does it reseat the reference? For a C++ reference, assignment does the former, and reseating is impossible. For Data, assignment is valid and assigns to the referenced object (e.g. a value in a dict or an item in a list), for DCR assignment is forbidden but reseating is possible via the reseat() member function.

In your first response, you have two examples of copy construction (which should be allowed, and is I think?), and the third one is a cast, which should be disallowed, I agree. I fully agree with the second response, that should be fixed and I'll do so.

[LourensVeen]

It's been a while, but I'm back to this and looking at all the above again, and I thought I was wrong but in the end what I wrote above seems to be the way to go anyway.

DataConstRef dcr1(1);
DataConstRef dcr2 = Data::list(1, 2, 3);
dcr1 = dcr2;

This is not allowed, and it shouldn't be, because it would overwrite the value referenced by dcr1, and that's not allowed through a DCR, that's the whole point of its constness. The problem here isn't that dcr2 is const, the problem is that dcr1 is const. We're not reseating the reference, we're assigning through it to the referenced value.

DataConstRef dcr = DataConstRef::list(1, 2, 3);

constructs a const reference to a newly created list (note that it's not an assignment!). This is not currently allowed, but it wouldn't hurt to add it. On the other hand, you can just use Data::list(1, 2, 3) instead with no difference to the semantics.

DataConstRef just_another_reference = dcr;

is copy-construction, should be allowed, and is allowed.

DataConstRef dcr = Data::list(1, 2);
Data casting_away_constness = dcr;

is copy-construction, but casts away constness. This should either copy the value or be disallowed, because if it doesn't copy then we've created an object from dcr through which we can modify the referenced value, and that shouldn't be allowed.

The mental model of Data and DataConstRef is a Python variable, so it's a primitive value or a reference, and that's what gets copied on assignment. The referenced value is never copied, so doing that here would be confusing. So we should disallow it, and we do.

DataConstRef dcr1 = DataConstRef::list(Args...);
DataConstRef dcr2 = DataConstRef::dict(Args...);

These don't exist, but could. As mentioned above, we have Data::list and Data::dict that work fine though.

    DataConstRef dcr1 = Data::list(1);                                          
    Data d1 = Data::list(dcr1);                                                 
    ASSERT_EQ(d1[0][0].as<int>(), 1);                                           
    d1[0][0] = 2;                                                               
    ASSERT_EQ(d1[0][0].as<int>(), 2);                                           
    ASSERT_EQ(dcr1[0].as<int>(), 2);                                                                                                                                                         

This compiles and passes, and it shouldn't compile because we've modified the value pointed to by dcr1, which should be impossible. That definitely needs to be fixed.

If that is fixed however, then it becomes impossible to take a value from a DataConstRef (e.g. something received previously) and put it into a list, because Data::list() won't accept it as an argument. And you can't make a Data::nils() and assign from the received DCR either, because Data::operator[] returns a Data and you're not allowed to assign to a Data from a DataConstRef.

So it's impossible to make a component that receives two messages on two incoming ports, glues them together into a list, and sends that on on an outgoing port, while that would be a very reasonable thing to do.

If we had DataConstRef::list() and DataConstRef::dict() then you could do that, by creating a const list containing the const received data. So they're not superfluous, we need them if we fix the second issue.

Okay, so same conclusion as above, we'll fix all the things mentioned in Maarten's last post :smile:.

[LourensVeen]

Released in 0.7.0.