Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am designing a class to store primitive types into byte buffer using predefined byte order. I am not going to use Boost.Serialization because I am working with plain types only and I need predefined binary structure without versions and things like that.

Here is draft of the design:

#ifndef SERIALIZER_H_
#define SERIALIZER_H_

/**
 * @file
 *
 * Set of byte-order independent value serializers.
 * New serializers can be added with template class specialization.
 *
 */

#include "types.hpp"

/**
 * Serializer generic interface.
 */
template<typename ValueType>
struct serializer
{
    /**
     * Parses value from raw bytes.
     *
     * @param  from byte buffer parse value from
     */
    static ValueType parse(uint8_t *from);

    /**
     * Writes value to raw byte buffer.
     *
     * @param  value value to write
     * @param  dest destination buffer
     */
    static void write(ValueType value, uint8_t *dest);
};

/**
 * Serializer specialization for single byte.
 */
template<>
struct serializer<uint8_t>
{
    static uint8_t parse(uint8_t *from)
    {
        return *from;
    }
    static void write(const uint8_t value, uint8_t *to)
    {
        *to = value;
    }
};

/**
 * Serializer specialization for 2-byte number.
 */
template<>
struct serializer<uint16_t>
{
    static uint16_t parse(uint8_t *from)
    {
        return (uint16_t)from[0] << 8 | from[1];
    }
    static void write(const uint16_t value, uint8_t *to)
    {
        to[0] = (value >> 8);
        to[1] = value & 0xff;
    }
};

/**
 * Serializer specialization for 4-byte number.
 */
template<>
struct serializer<uint32_t>
{
    static uint32_t parse(uint8_t *from)
    {
        return from[0] << 24 | from[1] << 16 | from[2] << 8 | from[3];
    }
    static void write(const uint32_t value, uint8_t *to)
    {
        serializer<uint16_t>::write(value >> 16, to);
        serializer<uint16_t>::write(value & 0xffff, to + 2);
    }
};

/**
 * Serializer specialization for 8-byte number.
 */
template<>
struct serializer<uint64_t>
{
    static uint64_t parse(uint8_t *from)
    {
        const uint32_t high = serializer<uint32_t>::parse(from);
        const uint32_t low = serializer<uint32_t>::parse(from + 4);
        return ((uint64_t) high << 32) | low;
    }
    static void write(const uint64_t value, uint8_t *to)
    {
        serializer<uint32_t>::write(value >> 32, to);
        serializer<uint32_t>::write(value & 0xffffffff, to + 4);
    }
};

/**
 * Packet abstraction.
 *
 * Packet has mutable head which moves on every read or write. Packet
 * overloads left/right shift operators to allow
 * serialization/deserialization operations chaining:
 *
 *     unit8_t buffer[80];
 *     packet p(buffer);
 *     p << obj.field << obj.other_field;
 *     // now obj fields serialized in the buffer
 *
 * Packet uses serializers for actual serialization/deserialization
 * logic.
 *
 * Single packet designed to be used for reading OR writing, not for
 * both operations interchaged. There is no way to move packet head
 * back.
 *
 * @see serializer
 */
class packet
{
  public:

    /**
     * Constructs packet from byte buffer pointer.
     *
     * @param  buf pointer to byte buffer
     */
    packet(uint8_t *buf);

    /**
     * Writes value to a packet.
     *
     * @param  p packet reference
     * @param  value value to write
     * @return reference to initial packet
     */
    template<typename ValueType>
    friend packet &operator<<(packet &p, ValueType value);

    /**
     * Reads value from a packet.
     *
     * @param  p packet reference
     * @param  target value buffer reference
     * @return reference to initial packet
     */
    template<typename ValueType>
    friend packet &operator>>(packet &p, ValueType &target);

    /**
     * Writes raw byte buffer to a packet.
     *
     * @param  bytes bytes to write
     * @param  length buffer length
     * @return reference to self
     */
    packet &write_bytes(const uint8_t *bytes, std::size_t length);

    /**
     * Reads `length` buffers from a packet.
     *
     * @param  dest target byte buffer
     * @param  length max buffer length
     * @return reference to self
     */
    packet &read_bytes(uint8_t *dest, std::size_t length);

    /**
     * Returns current head.
     *
     * @return current head
     */
    uint8_t *get_head() const;

    /**
     * Skips `n` bytes in input buffer.
     */
    packet &skip(std::size_t n);

    /**
     * Returns number of bytes written.
     *
     * @return number of bytes written 
     */
    off_t written() const;

    /**
     * Deallocates packet.
     */
    ~packet();

  private:
    uint8_t *_head;  ///< current head
    uint8_t *_start; ///< initial head
};

template<typename ValueType>
packet &operator<<(packet &p, ValueType value)
{
    serializer<ValueType>::write(value, p._head);
    p.skip(sizeof(ValueType));
    return p;
}

template<typename ValueType>
packet &operator>>(packet &p, ValueType &target)
{
    target = serializer<ValueType>::parse(p._head);
    p.skip(sizeof(ValueType));
    return p;
}

#endif /* SERIALIZER_H_ */

It looks like it does the right thing. What do you think about that?

share|improve this question
    
Why not use (the relatively standard) method of using htonl() and family (unfortunately for you it is network byte order (or big endian)). It is know to work and has optimal efficiency for the platform. –  Loki Astari Jan 17 '13 at 15:09
    
Yep, it looks like it's much better to provide generic native_to_network and network_to_native functions for uintN_t types which delegate their work to native functions. I just googled a bit and found that there were proposal for something like that in the Boost, but it isn't currently there (and probably will never be). Thank you! –  roman-kashitsyn Jan 18 '13 at 12:13
add comment

3 Answers 3

Here are my comments, some months later:

  • You don't need a destructor to your packet class. The class does not seem to own anything.
  • There is no mention in your packet class of the size (or the end) of the buffer. No check of size either. (consequently)
  • In your operator<<(the one in your self-answer), std::copy returns the new value that p.head should have. No need for p.skip(Size).
share|improve this answer
    
Thanks for replay ) Actually, I wrote a small C++ library based on code in the topic long time ago: github.com/roman-kashitsyn/encoding-binary –  roman-kashitsyn Jun 30 at 13:55
add comment

I ended up with something like that:

#ifndef PACKET_H_
#define PACKET_H_

#include <algorithm>
#include "types.hpp"
#include "endian.hpp"

class packet
{
  public:

    packet(uint8_t *buf);

    template<typename ValueType>
    friend packet &operator<<(packet &p, ValueType value);

    template<typename ValueType>
    friend packet &operator>>(packet &p, ValueType &target);

    packet &write_bytes(const uint8_t *bytes, std::size_t length);

    packet &read_bytes(uint8_t *dest, std::size_t length);

    uint8_t *get_head() const;

    packet &skip(std::size_t n);

    off_t written() const;

    ~packet();

  private:
    friend packet &operator<<(packet &p, uint8_t value);
    friend packet &operator>>(packet &p, uint8_t &target);
    uint8_t *_head;  ///< current head
    uint8_t *_start; ///< initial head
};

template<typename ValueType>
packet &operator<<(packet &p, ValueType value)
{
    const std::size_t Size = sizeof(ValueType);
    union {
        ValueType val;
        uint8_t arr[Size];
    } Copy;
    Copy.val = native_to_network(value);
    std::copy(Copy.arr, Copy.arr + Size, p._head);
    p.skip(Size);
    return p;
}

inline packet &operator<<(packet &p, uint8_t value)
{
    *(p._head++) = value;
    return p;
}

template<typename ValueType>
packet &operator>>(packet &p, ValueType &target)
{
    const std::size_t Size = sizeof(ValueType);
    union {
        ValueType val;
        uint8_t arr[Size];
    } Copy;
    std::copy(p._head, p._head + Size, Copy.arr);
    p.skip(Size);
    target = network_to_native(Copy.val);
    return p;
}

inline packet &operator>>(packet &p, uint8_t &target)
{
    target = *(p._head++);
    return p;
}

#endif /* PACKET_H_ */

native_to_network and network_to_native functions are overloaded for uint16_t, uint32_t, uint64_t and declared in the endian.hpp file with a lot of preprocessor derictives. It looks like it's working for me.

share|improve this answer
add comment

It just seems unnecessarily complicated. You're not doing anything to ensure binary compatibility between platforms so why not just write out the memory? Take a pointer to your data, cast it to char* and then read/write sizeof(ValueType) bytes from/to the file. There's no need to have specific template overrides for each type.

share|improve this answer
    
Actually, I am trying to write data in a way that allow me to read file written on one platform on any other platform, that's why I use integers of known size and unifying byte order to be always little-endian. –  roman-kashitsyn Jan 17 '13 at 9:14
    
You could programmatically check whether your platform is big or little endian and then reverse the byte order as appropriate. This would allow you to use a single bit of code for all of your data rather than a set of template specialisations. –  Jack Aidley Jan 17 '13 at 9:23
    
@roman-kashitsyn: Why little endian. The nearly d-facto standard is "network byte order" or big-endian. –  Loki Astari Jan 17 '13 at 15:13
    
Excuse me, it's my mistake. Big-endian, of course. It's actually implemented in the example. Thank you for your correction. –  roman-kashitsyn Jan 18 '13 at 11:17
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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