3
\$\begingroup\$

I'm currently creating a small server back-end. My goal is to write an (efficient) packet parser in C++17 using the command pattern. The packets are given by:

type: value in {0, 1, 255}
size: value in {0, 1, 65535}
data: array[size]

Each packet is processed by a packet_operator. The packet_registry is a packet_operator which simply calls the operator for the given packet type. I currently see no need for multiple, pipelined parsers for a single packet type. The code sample is given below and does compile without warnings.

packets.hpp

#pragma once

#include <array>
#include <memory>

// Test class without proper implementation.
class packet {

public:
    uint32_t m_type = 0;

    uint32_t type() const {
        return m_type;
    }

};


template<class ... ArgTypes>
class packet_operator {

public:
    virtual void operator()(const packet &, ArgTypes... args) = 0;

};


template<class ... ArgTypes>
class packet_registry : public packet_operator<ArgTypes...> {

public:
    using op_ptr = std::shared_ptr<packet_operator<ArgTypes...>>;

    explicit packet_registry(op_ptr op) {
        // Initialize registry with default processor
        for (size_t i = 0; i < m_operators.size(); i++)
            m_operators[i] = op;
    }

    void operator()(const packet &packet, ArgTypes... args) override {
        // Call processor for packet type
        (*m_operators[packet.type()])(packet, args...);
    }

    op_ptr &operator[](uint32_t i) {
        return m_operators[i];
    }

private:
    std::array<op_ptr, 256> m_operators;  // Type will always be in [0, 255]

};

main.cpp

#include <iostream>
#include <memory>

#include "packets.hpp"


class processor : public packet_operator<> {

    int m_id;

public:
    explicit processor(int id) : m_id(id) {
        // Empty
    }

    void operator()(const packet &packet) override {
        std::cout << "Processor " << m_id << ". Type: " << packet.type() << std::endl;
    }

};

void init(packet_registry<> &registry) {
    registry[1] = std::shared_ptr<packet_operator<>>(new processor(1));
    registry[2] = std::shared_ptr<packet_operator<>>(new processor(2));
}

int main() {
    // Create registry with default processor
    auto registry = packet_registry<>(
            std::shared_ptr<packet_operator<>>(new processor(0)));

    // Initialize packet processors
    init(registry);

    // Test packets with type 0, 1, 2 and 3
    packet p0, p1, p2, p3;
    p0.m_type = 0;
    p1.m_type = 1;
    p2.m_type = 2;
    p3.m_type = 3;

    registry(p0);
    registry(p1);
    registry(p2);
    registry(p3);

    return 0;
}

My main concern is my use of functors and shared pointers. Did I use those correctly? Additionally, my linter advised me to add the explicit keyword before the constructors. I've researched what it does, but do not really understand why those types of constructors would always need to be explicit.

I'd also like to know if there are any alternatives to my init method. I'd have to register the processors for every packet type here. I'd prefer some sort of self-registering processor. For example, in Java I could annotate all packet processors using a custom annotation. Those will then be added to the right registry using reflection. Maybe this is achievable through some sort of meta-programming?

Lastly, a more more general question: are there any alternative designs, given that I cannot change the protocol?

\$\endgroup\$
  • \$\begingroup\$ Not worthy as an answer, but capitalizing classes makes it easier to read \$\endgroup\$ – David Jun 2 '18 at 4:55
  • 1
    \$\begingroup\$ @David That is worthy as an answer. Reviewing code in comments is considered bad form here. \$\endgroup\$ – Daniel Jun 2 '18 at 9:18
  • \$\begingroup\$ @David I chose to stick with the already existing std/boost style and not impose some arbitrary style (e.g. Google) onto my project. \$\endgroup\$ – Rev Jun 2 '18 at 11:33
  • \$\begingroup\$ @Rev I wouldn't say arbitrary, but it is personal preference so it's more a suggestion. Having class names capitalized seems to make code easier to read (processor processor vs Processor processor), but it may depend on your background. \$\endgroup\$ – David Jun 2 '18 at 14:11
  • \$\begingroup\$ @David it actually plays quite well in large projects to name framework-y classes underscore separated lowercase and application classes camel case \$\endgroup\$ – bobah Jul 4 '18 at 6:22
2
\$\begingroup\$

The keyword explicit should be used with constructors taking a single argument in classes when automatic instantiation is not a universally valid operation (details and examples here).

The code review part

  • The packet_registry can just be an array of std::function (as you seem to be OK with paying for the virtual dispatch overhead) — std::array<std::function< packet const&, ArgTypes...>, 256>
  • processor-s, given they are created just once, are better be created by value. Then you will be able to have a very hackable syntax — registry[x] = [...](auto&& packet, auto&& arg) { ... actual code ...};
  • if for some reason you do need shared pointers, prefer make_shared over direct usage of the new operator — x = std::make_shared<type>(args...).
\$\endgroup\$
  • \$\begingroup\$ I'm aware of the (albeit small) VMT overhead, but didn't really see a way to implement this pattern without it. How would I go about implementing it without virtual methods? I'd still like to use class member functions to process packets. \$\endgroup\$ – Rev Jun 1 '18 at 12:34
  • \$\begingroup\$ I know of one approach to writing fast delegate explained here. There would be other implementations, but all base on a similar trick. \$\endgroup\$ – bobah Jun 2 '18 at 8:05
  • \$\begingroup\$ Whether virtual dispatch overhead is small or not depend primarily on the context. With local data and local code you can get very close to the theoretical throughput of the CPU pipeline(s). With virtual calls, especially per-message or per-field ones, you won’t get anywhere near. \$\endgroup\$ – bobah Jun 2 '18 at 8:09

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.