GobySoft / dccl

Dynamic Compact Control Language
Other
17 stars 13 forks source link

Enum codec skips not-enumerated integer values #45

Closed chrismurf closed 5 years ago

chrismurf commented 5 years ago

The current implementation of the Enum codec looks at the value_count() of the enumeration, which represents the current actual number of values of the enumeration - ie, the value_count for the following enumeration would be 4, not 15.

enum Fruit {
  FRUIT_UNKNOWN = 0;
  FRUIT_BANANA = 1;
  FRUIT_APPLE = 2;
  FRUIT_INVALID = 15;
}

This is the desired default behavior, but there are a number of use cases where effectively "pre-allocating" enumeration values would be useful - especially when dealing with enumerations that have large numbers of values (e.g. thousands).

chrismurf commented 5 years ago

I propose adding a boolean "packed" option to the option_extensions, which defaults to True and controls this behavior. When false, the codec would allocate based on the lowest and highest numerical values in the enumeration.

Duplicate values may cause undefined behavior in this use case. I think negative numbers should be fine - the codec can renormalize the values between the minimum and maximum values before transmission.

@tsaubergine - what is your opinion here? Any objections?

tsaubergine commented 5 years ago

Unless you see a use for the "packed" option for other Codecs, I would prefer "packed_enum". Otherwise I think this is a useful addition.

I think the Protocol Buffers behavior is fine for the multiple defined case ("If multiple values have this number, the first one defined is returned."):

const EnumValueDescriptor * 
    EnumDescriptor::FindValueByNumber(
        int number) const
Looks up a value by number.

Returns NULL if no such value exists. If multiple values have this number, the first one defined is returned.

Can you put up a PR that implements this and I'll review?

chrismurf commented 5 years ago

Happy to put together a PR, just figured I'd check your disposition first. Otherwise it was going to be a custom field codec ;-)