Reasons for implementing `sock_address_ref`

[anrddh]

First off, thanks for writing sockcpp! It's almost exactly what I was looking for in a modernized C++ sockets library.

I've been digging through the code, and I'm confused about why you've preferred to implement and use sock_address_refover using C++ references to sock_address.

[fpagliughi]

Sure. I'm happy to see that a few people have discovered this and started using it.

Honestly this library has its origins back to 2003 as a C++98 library for IPv4 in a larger embedded framework system. Some of the new code is evolving and could definitely be done differently, so I'm definitely open to suggestions.

The idea of the sock_address_ref is to have a quick, efficient "slice" into another object; kind of like the C++17 std::string_view. This can be generated rapidly from any of the different address objects.

I evolved the different address types as derived from their individual C structs. Since these don't derive from a common parent we can't use a pointer/reference to a base class for a generic address.

Perhaps that was a mistake and we should refactor the different addresses to be derived from sock_address and use that as the common base?

I'll certainly think on that.

[anrddh]

Having all the address classes derive from sock_address and store the C structs as member variables sounds like a good idea. I think this might lead to a clean solution for reducing code duplication between the separate implementations for IPv{4,6}. I'll think about it and see if I can make that work (your proposed solution for reducing duplication here in the README is templating, but inheritance feels like a cleaner way to achieve this TBH). All of this is very off-the-cuff of course.

On another note, I think there are some low-level coding issues that can be easily rectified and will not affect the interface. I'm not sure if you've considered these---if you have, these might very well not be "issues"---but here goes anyway.

• Calls to std::memcpy can be replaced by calls to std::copy. There is no performance loss (and maybe even minor performance gains to be had) as per [1], and using std::copy is arguably better C++ style. Many (all?) calls to std::memcpy can be replaced by calls to the copy-assignment operator if inheritance from C structs is replaced by storing the structs as members.
• Some calls to std::memset can be done away with entirely by leveraging "zero initialization". See [2].
• There are some C-style casts (for ex. line 177 of inet_address.h) that should be reinterpret_casts.

[1] https://stackoverflow.com/questions/4707012/is-it-better-to-use-stdmemcpy-or-stdcopy-in-terms-to-performance [2] https://en.cppreference.com/w/cpp/language/zero_initialization

[fpagliughi]

After thinking about it for an hour, I started to wonder if it's the best idea. The change would make it so that the memory used for any address would be the memory required for the largest address. For an IPv4 address, which is still likely the most common in use, that would be something like a 10x increase in memory usage.

I personally don't think I'd write anything with sockpp that juggles millions of addresses, but it seems like we'd be heading in the wrong direction, and someone, somewhere might try this!

I was trying to keep addresses as POD types, and avoiding vtables, but doing so also prevents us from an easy way of keeping a collection (i.e. std::vector) of heterogeneous addresses. And that might be handy. Hmm. I'm definitely undecided.

As for the other suggestions, they all sound good.

• I should definitely switch to std::copy(), but "old habits" and all that.
• I didn't know that zero initialization would work over C structs, but will definitely test it out.
• I use C++ casts when it's specific, like const_cast or static_cast, but I tend to be a bit lazy when it's a reinterpret_cast.

[anrddh]

After thinking about it for an hour, I started to wonder if it's the best idea. The change would make it so that the memory used for any address would be the memory required for the largest address. For an IPv4 address, which is still likely the most common in use, that would be something like a 10x increase in memory usage.

I was imagining that sock_address would become an abstract class without any member variables. The child classes would allocate storage via member variables as they need it.

[fpagliughi]

Yeah. I assume that there would still be a need for a class that does what the the current sock_address does - be able to hold and return an address of any type. That's something we might need for the return from base class calls when any address might be obtained, like socket::address() or peer_address().

But it might be worth trying this out and throwing it up in a branch to see how it feels.

[fpagliughi]

@anrddh Check out branch 'base_addr'. It does the following:

• Gets rid of sock_address_ref
• Makes sock_address an abstract base class
• Derives all the address classes from sock_address
• Consolidates a number of overloaded functions that took different forms of addresses to just take a const sock_address&
• Adds a new sock_address_any class that can contain any address, and is used by base classes that need a generic address.

In the long run, maybe we don't need sock_address_any, or if that's a good name for it. But it made for an easy update for now.

I haven't tested this very well or even tried to build or run the unit tests. I'm sure there's some more work to do.

But overall I like it. I think this would be a good way to proceed.

[anrddh]

Looks good! There are some build warnings. I'll open a pull request fixing those.

About classes like acceptor, etc., I actually do think templates might be the easiest way to reduce code duplication. The snippet below illustrates roughly what I'm thinking of:

template <typename acc_socket_t, typename acc_addr_t>
class acceptor {
//...
};

using tcp_acceptor = acceptor<tcp_socket, inet_address>;
using tcp6_acceptor = acceptor<tcp6_socket, inet6_address>;

The idea is to approximate the relationship between std::string and std::basic_string.

The downside is that the interface is a little more complicated. I'm actually not the biggest fan of this, and am only proposing it for the lack of better ideas.

[anrddh]

I've fixed the build warning and some unit test errors. See #9.

[anrddh]

About classes like acceptor, etc., I actually do think templates might be the easiest way to reduce code duplication. The snippet below illustrates roughly what I'm thinking of:

template <typename acc_socket_t, typename acc_addr_t>
class acceptor {
//...
};

using tcp_acceptor = acceptor<tcp_socket, inet_address>;
using tcp6_acceptor = acceptor<tcp6_socket, inet6_address>;

A quick and dirty proof of concept (that doesn't compile) for my idea with templates above: https://github.com/anrddh/sockpp/tree/templated-acceptor

[fpagliughi]

Ha, yes! I already started on the templates! But I got tired and went to sleep before I finished and committed it. I kept the base acceptor and connector classes the way they were and put a template on top to convert the addresses going in and out. I have an hour or two to work on it now. I'll merge the PR #9, finish what I started last night, push it, and then we can compare strategies.

[fpagliughi]

Yeah. Good. Overall I like where this went.

Like I said, I kept the concrete, generic base classes for connector and acceptor, then made a template around them. The Unix-domain sockets didn't exactly fit the templates, but were still able to use the base classes.

Let me know what you think. There are still some bugs, I'm guessing, and probably some new inconsistencies that have popped up. But overall, I think this is the way to proceed. I will merge this into the 'develop' branch in the next day or two, and will likely move to make a new v0.5 release when it seems stable.

[anrddh]

Are the base classes for acceptor and connector still being used elsewhere in the code?

[fpagliughi]

The unix_acceptor is derived directly from acceptor (not the template).

The unix_socket is not properly defined yet, and once it is, I'm not sure if it will bypass the template or not. Maybe.

[fpagliughi]

I looked at some of the "low-level" coding issues mentioned above. The zero initialization was nice and clean, where appropriate. There were also a couple of places where memcpy() was still being used instead of a simple member initialization. (A shakeout of refactoring the addresses).

I looked at std::copy(), but most of the places we're still using memcpy(), we're copying a specific number of bytes between two different address types which we know to be binary compatible.

std::copy() wants to copy a specific number of items between two iterators of the same type.

In this case it seems that memcpy() is actually a much better fit.

[fpagliughi]

I think that's it. Can we close this issue?

[anrddh]

Yep, looks good!

[fpagliughi]

Cool. This is a significant enough change that I will move to make it be the next release (v0.5).