4
\$\begingroup\$

I wrote two template functions to serialize and deserialize structs representing messages to be sent and received via a TCP socket:

/*
 * Convert a byte string into a packet.
 */
template <typename PacketType>
PacketType deserialize(std::string &buf, bool partialProcessing = false)
{
    PacketType packet;
    size_t bufSize = buf.length();
    size_t packetSize = sizeof packet;

    if (packetSize > bufSize)
        std::cerr << "Packet larger than buffer size. Partial read: " << (packetSize - bufSize) << std::endl;

    if (packetSize < bufSize && !partialProcessing)
        std::cerr << "Packet smaller than buffer size. Bytes unprocessed: " << (bufSize - packetSize) << std::endl;

    auto ptr = reinterpret_cast<char*>(&buf[0]);
    memcpy(&packet, ptr, (packetSize < bufSize) ? packetSize : bufSize);
    return packet;
}

/*
 * Convert a packet into a byte string.
 */
template <typename PacketType>
std::string serialize(PacketType &packet)
{
    auto ptr = reinterpret_cast<char*>(&packet);
    return std::string(ptr, ptr + sizeof packet);
}

The code currently works within a larger project quite well, but I am unsure as to whether it might produce memory leaks due to my usage of pointers and memcpy.

How can I improve this code?

Addendum Since some of the (great) answers seem to need more context, this is but an excerpt of this revision of a header file within the respective project. The template functions are used internally only i.e. do not constitute an external API.

\$\endgroup\$
7
\$\begingroup\$

Your code produces no memory leaks since you're not manually allocating and deleting memory.


Include all headers

Always include all the headers. In this case, you need <iostream>, string, and cstring.


Pass by const reference

You should pass your objects by const reference. For example, const std::string& buf and const PacketType& packet. You can use reinterpret_cast<const char*>.


Use std::memcpy instead of memcpy

If you're including <cstring> (which you should be since you're using C++), you should qualify your C function calls with the std namespace.

Including the C++-style header () guarantees that the entities will be found in namespace std and perhaps in the global namespace.


Compile time checks for POD structs

std::memcpy only does a shallow copy, so it only works on POD structs (i.e. C-style structs).

A Plain Old Data Structure in C++ is an aggregate class that contains only PODS as members, has no user-defined destructor, no user-defined copy assignment operator, and no nonstatic members of pointer-to-member type.

For example,

struct Example
{
    int a;
    std::string b;
};

is a non-POD struct since std::string is a non-POD type (has a non trivial copy constructor, non-trivial destructor).

You should make sure that PacketType is a POD struct. You can do this with a compile time check using static_assert.

You can check whether a type is POD, using is_pod type trait (deprecated in C++20) or using is_trivial && is_standard_layout. Here is a good description of what these terms mean and how they relate to POD types.

\$\endgroup\$
7
\$\begingroup\$

Here are some things that may help you improve your program.

Use all required #includes

Part of the interface is the #include files that are required, so those should be included here.

Make your code hard to abuse

Right now we're playing fast and loose with the assumption that memcpy will do the right thing, no matter what type is used for the template. That's a very dangerous assumption, and is easy to get wrong. The fact that you were unsure about its implication could be an indicator that perhaps something is not quite right.

Use return values or exceptions rather than printing

Printing to std::cerr is fine for debugging, but for production code, I'd probably prefer that either an exception is thrown or that a return value indicates the necessary information. At the very least, a parameter to turn printing on or off would be useful.

Let objects be expert about themselves

The PacketType class, I assume, is a placeholder for one of several types of specific Packet classes. The point to serialize is to "convert a byte string into a packet" according to the comment. It seems to me that creating a Packet would be the job of a Packet constructor. In this case, I'd suggest something like this:

class BigPacket {
public:
    BigPacket() { std::fill(data, data + size(), 0); }
    BigPacket(std::string& buf);
    explicit operator std::string() const;
    constexpr std::size_t size() const { return sizeof data; }
private:
    uint8_t data[79];
};

Now instead of writing code like this:

auto pkt = deserialize<BigPacket>(testvalue, true);

You can just write this:

auto pkt3 = BigPacket(testvalue);

All of the functionality is easily transferred into the constructor:

BigPacket::BigPacket(std::string& buf) {
    auto end{std::min(sizeof data, buf.size())};
    std::copy(buf.begin(), buf.begin() + end, data);
    if (buf.size() < sizeof data) {
        std::fill(&data[buf.size()], data + sizeof data, 0);
    }
}

Note that this follows the previous suggestion and printing is gone. If the caller wants to check for the stated conditions, all of the information is available to do those checks and printing if needed. It also explicitly sets each byte in the packet which is a useful security measure.

Use a conversion operator

You may have noticed the line:

explicit operator std::string() const;

Here's one way to implement it:

BigPacket::operator std::string() const {
    return std::string(reinterpret_cast<const char *>(&data[0]), sizeof data);
}

Now the serialize and deserialize are completely replaced by constructors and conversion operators:

std::string testvalue{"abcdefghijklmnopqrstuvwxyz"};
auto pkt = BigPacket(testvalue);
#if VERBOSE_LOGGING
if (pkt.size() > testvalue.size()) {
    std::cerr << "Packet larger than buffer size. Partial read: " << (pkt.size() - testvalue.size()) << '\n';
}
#endif
auto str = static_cast<std::string>(pkt);
std::cout << str << '\n';

Not only is this nice and neat, but it also means that more complex Packet types (such as those using shared pointers or dynamically sized buffers) can be accommodated with confidence and accuracy and a clean programming interface.

\$\endgroup\$
1
  • \$\begingroup\$ +1 for "Let objects be expert about themselves". Use of the OOP paradigm make code much more clean, robust and easier to extend later. \$\endgroup\$ – Jason Smith May 12 at 12:20
3
\$\begingroup\$

Adding to what Rish wrote, don't use std::endl. Just use \n in the string to indicate a newline.

\$\endgroup\$
4
  • \$\begingroup\$ I've come across this tip repeatedly now. From other sources I learned to use std::endl to indicate the end of a line. What I understand from the official docs is that endl executes an extra flush on the data stream, which may cost time and resources unnecessarily. Is this the reason behind your tip, or are there other/more issues with using endl? Thank you. \$\endgroup\$ – Richard Neumann May 11 at 19:07
  • 1
    \$\begingroup\$ That's the reason: it's unnecessary since flushing is done automatically anyway. \$\endgroup\$ – JDługosz May 11 at 22:53
  • \$\begingroup\$ For error logging purposes the extra flush can be beneficial as messages get written immediately. This is rather helpful for debugging as on a crash buffers might not get flushed. For other output, especially in tight loops, I'd also recommend using \n. \$\endgroup\$ – cg909 May 12 at 0:22
  • \$\begingroup\$ @JDługosz Flushing is done automatically if it's line-buffered. If you want to flush if, and only if, the stream is line buffered and you need to send a newline anyway, there is no point in calling a flushing function. First, that will flush unconditionally and you only wanted to flush if it's line buffered. Second, if it's line buffered, you just output a line, so you just flushed. So why are you calling a flushing function right after you flushed? \$\endgroup\$ – David Schwartz May 12 at 17:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.