MapProperty constructor does not work as intended
Description
camp::MapProperty
constructor currently does not work as intended. Its implementation is as below:
MapProperty::MapProperty(const std::string& name, Type elementType) :
Property(name, static_cast< ::camp::Type>(mappingType)),
m_elementType(elementType)
{
}
This static_cast< ::camp::Type>(mappingType)
result is unspecified. Indeed, camp::CustomType::mappingType
is 0x404
(), but camp::Type
is defined as follows.
enum Type
{
noType, ///< No type has been defined yet
boolType, ///< Boolean type (bool)
intType, ///< Integer types (unsigned/signed char short int long)
realType, ///< Real types (float, double)
stringType, ///< String types (char*, std::string)
enumType, ///< Enumerated types
arrayType, ///< Array types (std::vector, std::list, T[])
userType ///< User-defined classes
};
enum CustomType
{
mappingType = 0x404,
pairType
};
This can cause subtle bugs, as the user probably expects to have to check the possible values of camp::Type
and not things from other enum
s too.
Proposal
This is actually a bit more troublesome than it looks like. camp
is an external dependency, so we can't really just add the missing values to camp::Type
unless we ship a modified version with our needs.
I propose 4 approaches:
Approach 1:
- Construct the base object (parent) with
camp::Type::noType
- Brutally add a
MapProperty::type()
member function which will shadowProperty::type()
.- Store the value in a larger
enum
Approach 2:
- Do not inherit directly from
camp::Property
, but create an intermediatesight::camp::Property
, and then inherit from it.- In
sight::camp::Property
, use a largerenum
which is able to store both.- Either perform the same type of shadowing as described above or managed the
camp::Property
separately from the custom types that could be stored as an extension (so a separate member).
Approach 3:
Remove this class completely. Something tells me that it is actually not so useful and it's at line 73.
void MapProperty::setValue(const UserObject&, const Value&) const
{
//FIXME XXX TODO DO something
}
Approach 4:
""""Simply"""" ship a slightly modified version of
camp
with sight. Please don't do that.
I personally think either approach 2.
or approach 3.
would be the best.
Outcomes
Code that works.
Links / references
- MapProperty.cpp
- Compiler warning:
sight/libs/core/core/reflection/camp/MapProperty.cpp:36:47: warning: the result of the conversion is unspecified because ‘camp::mappingType’ is outside the range of type ‘camp::Type’ [-Wconversion]
36 | Property(name, static_cast< ::camp::Type>(mappingType)),
| ^~~~~~~~~~~