[BUG] sw::redis::Subscriber is move-assignable/constructable but does not have a default constructor

[RnMss]

Describe the bug
sw::redis::Subscriber has an empty state, but does not have a default constructor (sw::redis::Subscriber::Subscriber()) to make one.

sw::redis::Subscriber does not have Subscriber(), but does have Subscriber& operator= (Subscriber&& other) and Subscriber(Subscriber&& other) which is expected to make the other object an empty Subscriber.

If the class has an empty state, user should be allow to create one.

So that the following code could be valid.

class Foo {
public:
    do_something() {
         subscriber = redis_client.subscribe();
    }
private:
    sw::redis::Subscriber subscriber;
    ...
}

[RnMss]

#410

[sewenew]

but does have Subscriber& operator= (Subscriber&& other) and Subscriber(Subscriber&& other) which is expected to make the other object an empty Subscriber.

NO. You should not take the moved object as a valid Subscriber with empty state. Instead, it's a Subscriber with unspecified state, and you cannot use it except destroying it or re-assigning it. This is also how C++ move semantic works, and all STL objects behave (unless otherwise specified).

An empty Subscriber is not quite useful. If you need to define an empty subscriber, and assign it latter. You can define a unique_ptr<Subscriber> or shared_ptr<Subscriber> instead.

Regards

[RnMss]

Instead, it's a Subscriber with unspecified state, and you cannot use it except destroying it or re-assigning it.

A moved Subscriber has unspecified state means that ~Subscriber() has unspecified behavior.

This is also how C++ move semantic works, and all STL objects behave (unless otherwise specified).

No offense but you seem to have a misunderstanding about move semantics about non-copyable objects.

You think all STL objects behave like this, let's have a look at a typical example std::thread.

in c++11 specification https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3690.pdf

30.3.1.2 thread constructors [thread.thread.constr]

thread() noexcept;
Effects: Constructs a thread object that does not represent a thread of execution.

thread(thread&& x) noexcept;
Effects: Constructs an object of type thread from x, and sets x to a default constructed state.

And std::thread is not a special case,

Objects like std::string also have the same behavior that state after moved is specified.

// https://godbolt.org/z/dcv4G6cvn
#include <string>
#include <cassert>

using namespace std;

int main() {
    string a = "abc";
    string b{move(a)};

    assert(a.size() == 0);
    assert(a == string{});
    
    return 0;
}

/// https://godbolt.org/z/7nTz3T544
/// need c++20
#include <string>
#include <cassert>

using namespace std;

constexpr bool foo() {
    string a = "abc";
    string b{move(a)};
    return a == "";
}

static_assert(foo());

See also:
https://en.cppreference.com/w/cpp/language/rule_of_three

[sewenew]

A moved Subscriber has unspecified state means that ~Subscriber() has unspecified behavior.

No. The state is unspecified, but still valid, so that, as I mentioned above, you can only destroy it or re-assign it. Check tihs for detail.

You think all STL objects behave like this, let's have a look at a typical example std::thread

My comment above mentioned: This is also how C++ move semantic works, and all STL objects behave (unless otherwise specified).

It seems that you missed the sentence in the parentheses: unless otherwise specified. Some STL objects indeed ensure that the moved-from object will be in empty state (obviously, thread is one of them, and the standard specified that explicitly), but not all of them.

Some other examples:

• string's move constructor will leave the moved-from object valid but unspecified state (the 8th overload).
• vector's move constructor without allocator ensures that the move-from object is an empty vector (the 8th overload), however, the move constructor with allocator involved doesn't guarantee that (the 9th overload).

Objects like std::string also have the same behavior that state after moved is specified.

The standard you mention above explicit states that string's move constructor make the moved-from string in a valid but unspecified state.

21.4.2 basic_string constructors and assignment operators
basic_string(const basic_string& str);
basic_string(basic_string&& str) noexcept;
Effects: Constructs an object of class basic_string as indicated in Table 64. In the second form, str is left in a valid state with an unspecified value.

Since it's not specified, compilers are free to do anything, e.g. make the moved-from string empty. However, it's not guarantee by the standard, and the code is not portable, and you should not depend on the behavior. The underlying implementation might change in the future, or other compilers might have different behaviors.

Regards

[RnMss]

It seems that you missed the sentence in the parentheses: unless otherwise specified. Some STL objects indeed ensure that the moved-from object will be in empty state (obviously, thread is one of them, and the standard specified that explicitly), but not all of them.

Subscriber is a typically move-only class, so the std::thread is better example over std::string.

Even a moved Subscriber can be unspecified, I still see no reason not providing a default constructor.

[RnMss]

We almost missed the point, the point is about the default constructor rather than the moved state.

1. All the examples we talked about have default constructor. And that's how most STL class behave. (Whether or not the moved state is specified.)
2. Most (not completely sure but probably all. Feel free to correct me if there are exceptions) move-only STL class have default constructor, and the moved states are specified.

[sewenew]

This is some interesting discussion on a similar problem.

Regards

[RnMss]

Thanks for following this topic that could be kinda esthetics-based.

If I didn't get it wrong,

1. there are reasons not to use a default constructor != there is no reason to use a default constructor. You can provide a default constructor while users are free to choose to use or not.
2. From what you said above, I thought we have agreed on the designing philosophy of STL. I'm not sure if you agree but I think programmers should stick on the same convention if possible, rather than inventing new coding standard, unless there is some obvious defect on that.
3. From the implementation of this lib, a default constructor looks valid (and cheap).

Thanks.

[sewenew]

From what you said above, I thought we have agreed on the designing philosophy of STL.

However, the designing philosophy of STL never mentioned that an STL class must have a default constructor. For example, some members in std::exception family do not have default constructor, e.g. std::runtime_error, std::range_error. Because it does't make sense to have an empty runtime_error, i.e. no error info.

From the implementation of this lib, a default constructor looks valid (and cheap).

The problem is, so far, I do not find a default constructed Subscriber is valid. You can do nothing with it, except assigning a well-constructed Subscriber to it.

Sorry for the late reply, too busy these days...

Regards

[RnMss]

However, the designing philosophy of STL never mentioned that an STL class must have a default constructor. For example, some members in std::exception family do not have default constructor, e.g. std::runtime_error, std::range_error. Because it does't make sense to have an empty runtime_error, i.e. no error info.

Sorry but, IMO, exceptions aren't good example for that, while good examples should,

• be movable.
• maintain mutable states.
• manage external resources.

Thanks.

[wb2712]

In fact, the compiler will construct the default constructor as needed。
However, from the code programming specification, need to display the declaration constructor。
or use "=default" "
This is usually done due to the need for cross-platform to prevent compiler compilation results from errors

[RnMss]

In fact, the compiler will construct the default constructor as needed。

@wb2712 No. The default contructor won't be generated if there are user defined constructor or a default constructor cannot be generated (e.g. has a base class that can not be default constructed)

[witeyou]

An empty Subscriber is not quite useful. If you need to define an empty subscriber, and assign it latter. You can define a unique_ptr<Subscriber> or shared_ptr<Subscriber> instead.

Regards

How can I define a unique_ptr<Subscriber> or shared_ptr<Subscriber> instead?

When I using unique_ptr, I can't make or reset this pointer.

Sorry. I don't quite understand what you're saying, can you give me more details.

[sewenew]

@witeyou Try the following code:

auto r = Redis("redis://127.0.0.1");

std::unique_ptr<Subscriber> sub_uptr(new Subscriber(r.subscriber()));

auto sub_sptr = std::make_shared<Subscriber>(r.subscriber());

Regards

[RnMss]

@witeyou You can but I can't agree that it's good practice.
Either r.subscriber() should return std::unique_ptr<Subscriber> and remove or privatify the move-constructor/assignment,
or should a default constructor be added.
Wrapping a nullalbe object inside a unique_ptr just because we want to assign it later would only bring unwanted overhead. And a "rule-of-4" style class is valid but misleading.