LouisCharlesC / safe

Header only read/write wrapper for mutexes and locks.
MIT License
147 stars 11 forks source link

Semantics of safe::Safe::operator=() do not feel right #8

Closed LouisCharlesC closed 5 years ago

LouisCharlesC commented 5 years ago

The safe::Safe class template and specialization have an operator=() member function for assignment, but the semantics do not feel right:

safe::Safe<int> val;
val = val = 42;

does not compile because the operator=() accepts types that can be converted to int. The "val = 42" part is ok, but the "val = val" does not compile as safe::Safe is not convertible to int.

Two solutions:

  1. remove the return value of the operator=(). This does not feel right either as every other operator=() out there returns a reference to the assigned object.
  2. use a function like "set()" or "assign()" rather than operator=().

I prefer option # 2, and I prefer the name "set()". Any thoughts ?

offa commented 5 years ago

How about implementing it similar to std::unique_ptr? operator= assigns the value type and a reset() method the object itself.

safe::Safe<int> val;
val = 42; // ok - assign a new value

safe::Safe<int> val2;
val = val2; // error - operator=() disabled for this type
val.reset(val2); // ok - val

Enable operator=(Other&&) only for those types, where Other and ValueType are convertible (or explicit remove Safe from the overload set?)

LouisCharlesC commented 5 years ago

operator= assigns the value type and a reset() method the object itself.

It's the other way around, right ? operator=() only accepts a std::unique_ptr, whereas reset() accepts a pointer to the value type.

I still feel that any use of the operator=() is weird: disabling operator=() for type Safe means that calls to operator=() cannot be chained, which basically means the returned Safe& is useless.

Looking at the standard library, I see a few examples of member functions emplace() to replace the content of a container (e.g. std::any, std::variant, std::optional). Maybe I'll go for emplace().

offa commented 5 years ago

It's the other way around, right ? operator=() only accepts a std::unique_ptr, whereas reset() accepts a pointer to the value type.

Exact.

Looking at the standard library, I see a few examples of member functions emplace() to replace the content of a container (e.g. std::any, std::variant, std::optional). Maybe I'll go for emplace().

But key part of emplace() is to construct the element inplace. Thus it shouldn't take an actual value, but the parameters to construct it. Interestingly, both std::optional and std::any use operator= to assign value and type.

LouisCharlesC commented 5 years ago

I guess the assignment to the value and the type make sense for them. I don't like it for Safe because the operator/constructor would have to lock the mutex. But these operators can get implicitly called when passing arguments around and I don't like mutexes to be locked without the user knowing it. I don't know how strong an argument this is. What seems best to me is to delete copy and move operations and have a member emplace() function forward its arguments to the value's constructor, as you noted it should do.

LouisCharlesC commented 5 years ago

I went for assign(), rather than emplace(). I finally understood that emplace() does not make sense for Safe, whew. A member assign() function to replace undesirable assignment operators (operator=) seems straightforward enough for me. Pull request #14 has been submitted with the new interface.