These are good questions!
1. You should indeed have the copy constructor & assignment operator deleted.
The "rule of five" tells you to specifically define a copy c'tor and assignment operator - but it doesn't tell you that you have to make the available. It is a perfectly valid choice to decide to not allow your object to be copied or non-move-assigned - only moved. An example of this: std::unique_ptr
.
It's also what I would recommend in your case, because:
- Like you said, duplicating a file descriptor is weird and unexpected.
- It's not obvious to the user of your class what the copy behavior should be.
- There don't seem to be - AFAICT - common scenarios in which you would copy, rather than move or pass by reference, a TCP socket.
2. Consider using std::optional
to indicate "no valid value" or "missing" or "none"
In C (and the C system call bindings on Unix-like systems), it is a convention to use the -1 value for an invalid/missing file descriptor is a convention. We know that the int
type is actually larger than the actual space of possible file descriptor values, so we use a junk value, which we assume the OS never uses, to indicate "no valid value". Now, this works fine; and you could choose to, say, define a static class constant:
static constexpr const int no_file_descriptor { -1 };
and then write:
close(_s);
_s = other._s;
other._s = no_file_descriptor;
As @HolyBlackCat suggests, you must get _s
to be closed somehow. You could also just swap the two descriptors, but I find that to contradict the element of least surprise.
but you might want to consider the more general (though less space-efficient) solution, which is the std::optional<T>
type template. It is intended for exactly your case: Either holding some value of type T
(in your case, int
), or holding some indication of "no value". Using an optional, you could write:
close(_s.get());
_s = other._s;
other._s = std::nullopt;
You'll still need to write your move assignment and move construction code, unfortunately (thanks @CassioRenan for noticing these are both necessary).
Other suggestions
- Don't use a plain
int
; either find a type definition of a file descriptor from some library you're using, or if you have no definition to borrow, have using file_descriptor = int;
or using file_descriptor_index = int
.
_s
is a bad member name. Use something more explicit, e.g. descriptor_index_
or posix_descriptor_index_
.
- Always check the return value of library/system calls! And handle errors.
The modified code
class tcp_socket {
protected:
void close_if_neccessary() {
constexpr auto socket_close_failed { -1 };
if (descriptor_index_.has_value()) {
auto retval = close(descriptor_index_.value());
if (retval == socket_close_failed) {
// throw something here, e.g.:
// throw std::system_error(errno, std::system_category(), "close()");
}
};
public:
using file_descriptor_index = int;
tcp_socket() : descriptor_index_() {
constexpr auto socket_creation_failed { -1 };
auto retval = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
if (retval == socket_creation_failed) {
// throw std::system_error(errno, std::system_category(), "socket()");
}
descriptor_index = retval;
};
tcp_socket(const tcp_socket& other) = delete;
tcp_socket& operator=(const tcp_socket& other) = delete;
tcp_socket& operator=(tcp_socket&& other) noexcept {
if (other.descriptor_index_ != descriptor_index_) {
close_if_neccessary();
descriptor_index_ = other.descriptor_index_;
other.descriptor_index_ = std::nullopt;
}
return this;
};
tcp_socket(tcp_socket&& other) noexcept :
descriptor_index_(other.descriptor_index_)
{
other.descriptor_index = std::nullopt;
}
~tcp_socket() {
// You might want to wrap this in a try-catch, since
// destructors shouldn't throw. Otherwise you're risking
// a double-exception and immediate program termination.
close_if_necessary();
}
protected:
std::optional<file_descriptor_index> descriptor_index_;
};
See this StackOverflow question about the weird exception code in the comments:
tcp_socket_ref
which act as an observer to the socket. Much like you can use raw pointers to observe the value of astd::unique_ptr
\$\endgroup\$