breese / trial.protocol

Network wire protocols
11 stars 4 forks source link

[dynamic] enum support? #29

Closed breese closed 6 years ago

breese commented 6 years ago

Enumerations have their own distinct type, so they cannot be added to dynamic::variable. However, they have an integer as the underlying type, so they could be stored as such.

I am considering adding direct support for storing and retrieving enumerations.

For example:

enum class light { off, on };

// Create variable with on state.
dynamic::variable data(light::on)

// Check if stored value is an enum.
// Notice: this will succeed for any integer as well.
if (data.is<light>())
{
  // Retrieve the stored value as an enumerator.
  // Notice: the stored value may not match a valid enumerator.
  auto value = data.value<light>();
}

The disadvantage of this, is that the use can store an arbitrary integer, and then retrieve it as an enum which may not have an enumerator with the given integer value. This will result in undefined behaviour.

vinipsmaker commented 6 years ago

and then retrieve it as an enum which may not have an enumerator with the given integer value. This will result in undefined behaviour.

IOW, it's not useful to read data from network. It's only useful for internal message processing. If this is the case, I think the user would be better off with explicit casts.

However, this solution could be iterated and refined. I'm more concerned about another face of this feature.

If C++ gains reflection support, you could convert to/from enums using strings instead of integers (the previous example would store "light::on" or "on"). And retrieval could check if enum is “out of bounds”. Thoughts?

breese commented 6 years ago

Good point about serialization.

If an enum should be serialized as strings, dynamic::variable should contain those strings. Likewise if an enum should be serialized as the underlying type, dynamic::variable should contain the underlying type.

It would therefore be better if we use the dynamic::convert function to map between enums and dynamic::variable. By default this could map to the underlying type, but users can customize their enum type to map them to strings. dynamic::variable itself would then continue to know nothing about enums.

This extension should be easy to make. I just need to pull the converter overload structs out of the detail namespace.

vinipsmaker commented 6 years ago

This template may be specialized for a user-defined type to indicate that the type is eligible for std::error_code and std::error_condition automatic conversions.

I think the same can be done here with the dynamic::convert function. Explicit opt-in.

breese commented 6 years ago

That is a good idea.

As convert goes from being a couple of helper functions to a more general framework for converting between dynamic::variable and other C++ data types, I am considering refactoring the names.

I would like to put all conversion functionality into a dynamic::convert namespace. The current dynamic::convert<T> function could be renamed to something along the lines of:

auto dynamic::convert::to<T>(dynamic::variable) -> T;
auto dynamic::convert::from<T>(T) -> dynamic::variable;

Maybe there should also be a generic dynamic::convert::convert<T>() function for meta-programming purposes.

The trait you are suggesting could then become something like:

template <typename T>
dynamic::convert::use_underlying_type<T>;

And conversion of custom types:

template <lots of stuff>
struct dynamic::convert::overloader
{
   // More stuff
};
breese commented 6 years ago

I have experimented with this, and am quite happy with the results.

  1. All conversion is put into the dynamic::convert namespace.
  2. The dynamic::convert<T>(U) function is renamed to dynamic::convert::into<T>(U)
  3. Conversion of custom types can be added by specializing the dynamic::convert::overloader template.
  4. Conversion of enums can also be done with by specializing the dynamic::convert::use_underlying_type<T> trait.

Any objections to this change?

vinipsmaker commented 6 years ago

Any objections to this change?

1, 2 and 4 are fine.

I'll look into how to use 3 and ping back.

vinipsmaker commented 6 years ago

I'll look into how to use 3 and ping back.

I don't see dynamic::convert::overloader in the recent commits. Is it the same as https://github.com/breese/trial.protocol/blob/78549c26567e15fb297393ec24a5f851f4fa8d05/include/trial/dynamic/detail/variable.ipp#L195?

breese commented 6 years ago

I have just added the code on the feature/enum branch.

You can find an example of customization in test/dynamic/convert/enum_suite.cpp. Search for string_enumeration.

vinipsmaker commented 6 years ago

Okay. Nice design.

breese commented 6 years ago

Merged into develop