search

fpagliughi / sockpp

Modern C++ socket library.
BSD 3-Clause "New" or "Revised" License
782 stars 126 forks source link

Fixed broken compilation of examples on Windows/msvs2015u3 #43

Closed tsilia closed 4 years ago

tsilia commented 4 years ago

Fixes #42

tsilia commented 4 years ago

Hello @fpagliughi

I'd also recommend to change signatures of datagram_socket_tmpl::send_to and datagram_socket_tmpl::recv_from so that they accept uint8_t* rather than void*. IMO, this would be more type-safe because this way it requires an explicit cast to uint8_t* when calling those methods, instead of relying on silent automatic cast to void*. Besides, void* looks more C-way than C++ 😉 (The same applies to stream_socket::read and the likes).

But since this changes API and potentially breaks code (I checked that examples do compile though), I'm only suggesting that in a comment rather than in a PR. I'll update the PR if you're OK with that.

fpagliughi commented 4 years ago

Cool. Thanks. I'll get this tested over the next few days, but it doesn't look like anything that would cause a problem.

As for changing the signature of the send/recv functions, no, I don't think that's worth doing a breaking change. A lot of people would then need to update their projects to adapt the change (myself included) and not really get anything for it. Plus, when I've tried this sort of thing in the past, it's created a lot of problems with string data. Since different systems/compilers treat characters as signed or unsigned, it doesn't always port well.

fpagliughi commented 4 years ago

OK, a quick look, and it appears that uniform_int_distribution<char> is definitely not supported by the C++ standard. So it's good to get that out of there.

But as for using std::array<> as a read buffer, I am unconvinced. Since the size() of an array is fixed, it doesn't actually tell you the number of valid items in the array, but rather just the capacity. I think it would be a bad example to use array.

This library popped out of a bigger one that had a bunch of different type definitions, including an I/O buffer class that could be returned from a read() operation. In that case it would be properly sized from the read. Something like that would be better and perhaps more "C++", but I think that's a little beyond the scope of this library.

tsilia commented 4 years ago

The std::array is just a thin wrapper over a C-ctyle array, but no problem, I've updated the PR. The cast to void* is there in order to avoid ambiguity (the details are in #42).

fpagliughi commented 4 years ago

Oops. I missed that the casting was required for MSVS. OK, we'll get that fixed.