christianrauch / msp

Implementation of the MultiWii Serial Protocol (MSP) for MultiWii and Cleanflight flight controller
http://www.multiwii.com/wiki/index.php?title=Multiwii_Serial_Protocol
GNU Lesser General Public License v3.0
73 stars 26 forks source link

Value assignment via cast to type #33

Closed christianrauch closed 5 years ago

christianrauch commented 5 years ago

This PR implements a cast operator to allow the easy access of the underlying raw data in class Value. E.g.

Value<float> v = 13.4;
float data = v;

This clashes with the bool cast operator, that is used to test if a value is set. To support boolen values (i.e. Value<bool>) this operator is removed and one needs to check if a value is set using the set() method.

christianrauch commented 5 years ago

@humanoid2050 Can you check if this clashes with the usage in your original pull request? Specifically because the bool cast for checking if a value is set has been removed. Any cast operation on Value<T> now reveals the original value instead of providing true/false set attribute.

humanoid2050 commented 5 years ago

As long as you got all instances of using the old operator bool() const then I think this should be fine. I assume you removed the function signature and then fixed the errors that emerged. If you want to be paranoid, I think you could make the new operator T() const { return data.first; } method explicit and force any automatic conversions to announce themselves when compiling. Otherwise I'm ok with this.

christianrauch commented 5 years ago

Thanks for the hint. Making the cast explicit actually revealed one compiler error where rxConfig.receiverType was accessed to check if the value is set. Because the feature of optional message fields was introduced with the MSP2 pull request, I thought you might remember if accessing the set state of a value via the bool cast is used more often in the code.

Since the Value<> is used quite often in the code, and not only for optional fields, a full audit of the code is quite time consuming. I've not seen issues with my code, but I may consider to use the Value<> class only for optional fields and revert common message fields to their original type to make sure that testing for set via the old bool cast does not interfere with a normal value cast.

humanoid2050 commented 5 years ago

I do remember thinking about the potential conflict between using the bool operator to check if the value is set and having a bool data type stored in a value. I think there weren't any bool data values in the code, so it wasn't a problem in the code base. I'm reasonably confident that if you did the conversion by removing the original bool operator and then changing everything that broke to the set method, I would expect that you got everything correct.