commschamp / comms_champion

C++(11) library to implement and tools to monitor binary communication protocols
https://commschamp.github.io
Mozilla Public License 2.0
242 stars 16 forks source link

VariantHandler #3

Closed ghost closed 6 years ago

ghost commented 6 years ago

Hi,

Thank you for you great job, the library design is very smart and we are projecting to use it in commercial projects...

Just one issue we cannot solve with Variant and VariantHandler (following documentation exemples):

using MyFieldBase = comms::Field<comms::option::LittleEndian>;
using Value1 = comms::field::IntValue<MyFieldBase, std::uint8_t>;
using Value2 = comms::field::IntValue<MyFieldBase, std::uint32_t>;
using Value3 = comms::field::String<MyFieldBase,comms::option::SequenceSizeFieldPrefix<comms::field::IntValue<MyFieldBase,std::uint8_t>>>;

enum class KeyId : std::uint8_t
{
    Key1,
    Key2,
    Key3,
    NumOfValues
};

template <KeyId TId>
using KeyField = comms::field::EnumValue<MyFieldBase,KeyId,comms::option::DefaultNumValue<(int)TId>,comms::option::ValidNumValueRange<(int)TId, (int)TId>,comms::option::FailOnInvalid<>>;

using Key1 = KeyField<KeyId::Key1>;
using Key2 = KeyField<KeyId::Key2>;
using Key3 = KeyField<KeyId::Key3>;

using Property1 = comms::field::Bundle<MyFieldBase, std::tuple<Key1, Value1> >;
using Property2 = comms::field::Bundle<MyFieldBase, std::tuple<Key2, Value2> >;
using Property3 = comms::field::Bundle<MyFieldBase, std::tuple<Key3, Value3> >;

struct MyVariant
: public comms::field::Variant<MyFieldBase,std::tuple<Property1,Property2,Property3>,comms::option::DefaultVariantIndex<0>>
{
    COMMS_VARIANT_MEMBERS_ACCESS(prop1, prop2, prop3);
};

struct MyVariantHandler
{
    void operator()(Property1& prop) {}
    void operator()(Property2& prop) {}
    void operator()(Property3& prop) {}
};

void handleVariant(MyVariant& var)
{
    var.currentFieldExec(MyVariantHandler());
}

This code doesn't compile with the following error : error: no member named 'operator()' in 'MyVariantHandler' func_.template operator()<TIdx>(*(reinterpret_cast<TField*>(storage_)))

I'm using Xcode with LLVM 9.0, clang-900.0.39.2. I have also try with gcc 5.4.1 with the same error. It seems the template resolution doesn't match, but how to implement MyVariantHandler to make this work?

Regards,

arobenko commented 6 years ago

Hi, Thanks for your report. I looks like the documentation for Variant field got a bit outdated. Sorry about that, I'll fix it in the next release (which I plan to do until the end of the year).

At some stage I decided to add also a compile time information about the index of the handled field within the held tuple. Just add template prior to every operator() definition in your handler class. If you don't need it, you may either ignore it or static_assert on it.

struct MyVariantHandler
{
    template <std::size_t TIdx>
    void operator()(Property1& prop) 
   {
       static_assert(TIdx == 0, "Wrong index");
   }

    template <std::size_t TIdx>
    void operator()(Property2& prop) {...}

    template <std::size_t TIdx>
    void operator()(Property3& prop) {...}
};

You can also look at test70 in my unit-tests for fields for the example.

By the way, may I ask you why you decided to use Variant field in your protocol? It's not very efficient. The complexity of the "read" operation is O(n), while the complexity of "dispatch to handling" operation is O(log(n)). I had to introduce it in order to support unordered key/value pairs of various properties in MQTTv5 protocol (my work is here), but I would not choose such data layout in the protocols I'm defining. Are you going to define a new internal protocol that you are in complete control of what's going inside, or you have to implement already defined protocol, that requires you to use the Variant field to support it? If you don't want to post your answer in the public ticket, please send me a direct e-mail to arobenko@gmail.com.

Regards, Alex

ghost commented 6 years ago

Thank you Alex, works fine.