| Message ID | 20190621161401.28337-2-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Fri, Jun 21, 2019 at 05:13:53PM +0100, Kieran Bingham wrote: > To facilitate passing typed data generically, implement a class which stores > the type and value as efficiently as possible. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/meson.build | 1 + > include/libcamera/value.h | 63 ++++++++++ > src/libcamera/meson.build | 1 + > src/libcamera/value.cpp | 226 ++++++++++++++++++++++++++++++++++ > 4 files changed, 291 insertions(+) > create mode 100644 include/libcamera/value.h > create mode 100644 src/libcamera/value.cpp > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > index 201832105457..eb2211ae1fc3 100644 > --- a/include/libcamera/meson.build > +++ b/include/libcamera/meson.build > @@ -12,6 +12,7 @@ libcamera_api = files([ > 'signal.h', > 'stream.h', > 'timer.h', > + 'value.h', > 'version.h', > ]) > > diff --git a/include/libcamera/value.h b/include/libcamera/value.h > new file mode 100644 > index 000000000000..00c5d53d5cc0 > --- /dev/null > +++ b/include/libcamera/value.h > @@ -0,0 +1,63 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * value.h - Abstract value handling > + */ > + > +#include <map> > + > +#ifndef __LIBCAMERA_VALUE_H__ > +#define __LIBCAMERA_VALUE_H__ > + > +namespace libcamera { > + > +enum ValueType : uint8_t { Do we need an explicit uint8_t here ? It won't save memory as the next member of the Value class will be aligned to a 64-bits boundary. > + ValueNull, Let's call it ValueUnset or ValueNone. Null has a well established meaning. > + ValueBool, > + ValueInteger, > + ValueInteger64, > + ValueString, > +}; > + > +class Value I think I would call this ControlValue for now, and then expand it to a more generic solution if needed, instead of trying to be too generic to start with. > +{ > +public: > + Value(); > + Value(bool value); > + Value(int value); > + Value(const char *value); > + Value(const std::string &value); > + > + ValueType type() const { return type_; }; > + bool isNull() const { return type_ == ValueNull; }; isUnset() ? > + > + void set(bool value); > + void set(int value); > + void set(int64_t value); This is asking for trouble, you will often end up with the wrong integer type without even noticing, unless you can remember and apply flawlessly the C++ integer types implicit conversions rules. > + void set(const char *value); > + void set(const std::string &value); > + > + bool getBool() const; > + int getInt() const; > + int getInt64() const; This should return a 64-bit integer. > + std::string getString() const; > + > + std::string toString() const; > + > +private: > + ValueType type_; > + > + union { > + bool bool_; > + int integer_; > + int64_t integer64_; > + }; > + std::string string_; As you've noticed, usage of an std::string makes it impossible to make the string_ member part of the value union, which reduces memory and CPU efficiency. Every integer value will need to construct an std::string. For this reason I would store the data for complex types as a pointer instead, and allocate it dynamically. It could be a pointer to a std::string, or just a const char *. The latter would likely be more efficient. > +}; > + > +std::ostream &operator<<(std::ostream &stream, const Value &value); > + > +} /* namespace libcamera */ > + > +#endif /* __LIBCAMERA_VALUE_H__ */ > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > index 68c7ce14b5b4..8e68373118df 100644 > --- a/src/libcamera/meson.build > +++ b/src/libcamera/meson.build > @@ -27,6 +27,7 @@ libcamera_sources = files([ > 'v4l2_device.cpp', > 'v4l2_subdevice.cpp', > 'v4l2_videodevice.cpp', > + 'value.cpp', > 'version.cpp', > ]) > > diff --git a/src/libcamera/value.cpp b/src/libcamera/value.cpp > new file mode 100644 > index 000000000000..f713907b38dd > --- /dev/null > +++ b/src/libcamera/value.cpp > @@ -0,0 +1,226 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * value.cpp - Abstract value handling > + */ > + > +#include <limits.h> > +#include <sstream> > +#include <string> > + > +#include <libcamera/value.h> Please move this as the first include to ensure that it compiles without depending on any implicitly included headers. > + > +#include "log.h" // For ASSERT() You don't need to keep the comment. > + > +/** > + * \file value.h > + * \brief Handles variant value types as an abstract object > + */ > + > +namespace libcamera { > + > +/** > + * \enum ValueType > + * Determines the type of value represented by a \a Value > + * \var ValueNull > + * Identifies an unset value > + * \var ValueBool > + * Identifies controls storing a boolean value > + * \var ValueInteger > + * Identifies controls storing an integer value > + * \var ValueString > + * Identifies controls storing a string value You're missing the 64-bit integer. > + */ > + > +/** > + * \class Value > + * \brief Abstract a value for a common data exchange > + */ > + > +/** > + * \brief Construct an empty Value. > + * The Value must be \a set() before being used. > + */ > +Value::Value() > + : type_(ValueNull) > +{ > +} > + > +/** > + * \brief Construct a Boolean Value > + * \param[in] value Boolean value to store > + */ > +Value::Value(bool value) > + : type_(ValueBool), bool_(value) > +{ > +} > + > +/** > + * \brief Construct an integer Value > + * \param[in] value Integer value to store > + */ > +Value::Value(int value) > + : type_(ValueInteger), integer_(value) > +{ > +} > + > +/** > + * \brief Construct a string Value > + * \param[in] value String representation to store > + */ > +Value::Value(const char *value) > + : type_(ValueString), string_(value) > +{ > +} > + > +/** > + * \brief Construct a string Value > + * \param[in] value String representation to store > + */ > +Value::Value(const std::string &value) > + : type_(ValueString), string_(value) > +{ > +} > + > +/** > + * \fn Value::type > + * \brief Return the type of value represented by the object > + */ > + > +/** > + * \fn Value::isNull > + * \brief Determines if the Value is initialised s/Determines/Determine/ > + * \return true if the value type is ValueNull, false otherwise s/true/True/ > + */ > + > +/** > + * \brief Set the value with a boolean > + * \param[in] value Boolean value to store > + */ > +void Value::set(bool value) > +{ > + type_ = ValueBool; > + bool_ = value; > +} > + > +/** > + * \brief Set the value with an integer > + * \param[in] value Integer value to store > + */ > +void Value::set(int value) > +{ > + type_ = ValueInteger; > + integer_ = value; > +} > + > +/** > + * \brief Set the value with a 64 bit integer > + * \param[in] value Integer value to store > + */ > +void Value::set(int64_t value) > +{ > + type_ = ValueInteger64; > + integer64_ = value; > +} > + > +/** > + * \brief Set the value with a string representation > + * \param[in] value String value to store > + */ > +void Value::set(const char *value) > +{ > + type_ = ValueString; > + string_ = value; > +} > + > +/** > + * \brief Set the value with a string representation > + * \param[in] value String value to store > + */ > +void Value::set(const std::string &value) > +{ > + type_ = ValueString; > + string_ = value; > +} > + > +/** > + * \brief Get the boolean value. > + * > + * The Value type must be Boolean. > + */ > +bool Value::getBool() const > +{ > + ASSERT(type_ == ValueBool); That's quite dangerous... Isn't there a large risk that the value will be set to a type and retrieved using a different but compatible type, and that the issue would only be caught at runtime, possibly in uncommon cases ? For instance consider the following code Value v; if (likely(condition)) value.set(3); else value.set(0x100000000ULL); ... int i = v.getInt(); This will cause a crash in the unlikely event that condition is false, and could get unnoticed through testing. An explicit set API with type names may help there. Given that the Value class will be used for controls, I think we'll have to make sure we don't crash, invalid types will need to be caught and reported. We could also decide than an invalid type would result in the control taking a default value instead of crashing (but then this should be logged). > + > + return bool_; > +} > + > +/** > + * \brief Get the integer value. > + * > + * The Value type must be Integer. > + */ > +int Value::getInt() const > +{ > + ASSERT(type_ == ValueInteger); > + > + return integer_; > +} > + > +/** > + * \brief Get the 64bit integer value. > + * > + * The Value type must be Integer64. > + */ > +int Value::getInt64() const > +{ > + ASSERT(type_ == ValueInteger64); > + > + return integer64_; > +} > + > +/** > + * \brief Get the string value. > + * > + * The Value type must be String. > + */ > +std::string Value::getString() const > +{ > + ASSERT(type_ == ValueString); > + > + return string_; > +} > + > +/** > + * \brief Prepare a string representation of the Value > + */ > +std::string Value::toString() const > +{ > + switch (type_) { > + case ValueNull: > + return "<NULL>"; > + case ValueBool: > + return bool_ ? "True" : "False"; > + case ValueInteger: > + return std::to_string(integer_); > + case ValueInteger64: > + return std::to_string(integer64_); > + case ValueString: > + return string_; > + } > + > + /* Unreachable */ > + return "<ValueType Error>"; Do you need this ? Does the compiler complain when the switch-case handles all the cases ? > +} > + > +/** > + * \brief Provide a string stream representation of the Value \a value to > + * the \a stream. It's not a string stream, it's an output stream (and you should include ostream instead of sstream). > + */ > +std::ostream &operator<<(std::ostream &stream, const Value &value) > +{ > + return stream << value.toString(); > +} > + > +} /* namespace libcamera */
Hi Laurent, On 22/06/2019 23:39, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Fri, Jun 21, 2019 at 05:13:53PM +0100, Kieran Bingham wrote: >> To facilitate passing typed data generically, implement a class which stores >> the type and value as efficiently as possible. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> include/libcamera/meson.build | 1 + >> include/libcamera/value.h | 63 ++++++++++ >> src/libcamera/meson.build | 1 + >> src/libcamera/value.cpp | 226 ++++++++++++++++++++++++++++++++++ >> 4 files changed, 291 insertions(+) >> create mode 100644 include/libcamera/value.h >> create mode 100644 src/libcamera/value.cpp >> >> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build >> index 201832105457..eb2211ae1fc3 100644 >> --- a/include/libcamera/meson.build >> +++ b/include/libcamera/meson.build >> @@ -12,6 +12,7 @@ libcamera_api = files([ >> 'signal.h', >> 'stream.h', >> 'timer.h', >> + 'value.h', >> 'version.h', >> ]) >> >> diff --git a/include/libcamera/value.h b/include/libcamera/value.h >> new file mode 100644 >> index 000000000000..00c5d53d5cc0 >> --- /dev/null >> +++ b/include/libcamera/value.h >> @@ -0,0 +1,63 @@ >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> +/* >> + * Copyright (C) 2019, Google Inc. >> + * >> + * value.h - Abstract value handling >> + */ >> + >> +#include <map> >> + >> +#ifndef __LIBCAMERA_VALUE_H__ >> +#define __LIBCAMERA_VALUE_H__ >> + >> +namespace libcamera { >> + >> +enum ValueType : uint8_t { > > Do we need an explicit uint8_t here ? It won't save memory as the next > member of the Value class will be aligned to a 64-bits boundary. True, but ValueType might be used in other locations too? Although then they'd only be used for checking the type - so storage isn't really an issue there either. My actual reason for playing around with specifying the types on enums was to explore the effects on the ABI. The ValueType enum should certainly never exceed 255 entries, which is one benefit of being explicitly smaller - but I do think on this occasion it is completely redundant so I'll just remove it now. > >> + ValueNull, > > Let's call it ValueUnset or ValueNone. Null has a well established > meaning. I started with ValueNone ... but moved to ValueNull ... but I'm happy to go back again :-) > >> + ValueBool, >> + ValueInteger, >> + ValueInteger64, >> + ValueString, >> +}; >> + >> +class Value > > I think I would call this ControlValue for now, and then expand it to a > more generic solution if needed, instead of trying to be too generic to > start with. And this whole class came from ControlValue > >> +{ >> +public: >> + Value(); >> + Value(bool value); >> + Value(int value); >> + Value(const char *value); >> + Value(const std::string &value); >> + >> + ValueType type() const { return type_; }; >> + bool isNull() const { return type_ == ValueNull; }; > > isUnset() ? Well if it's not Null anymore, shouldn't it be isNone? Or do you prefer (Un)'set' to mirror the set() call. >> + >> + void set(bool value); >> + void set(int value); >> + void set(int64_t value); > > This is asking for trouble, you will often end up with the wrong integer > type without even noticing, unless you can remember and apply flawlessly > the C++ integer types implicit conversions rules. Hrm... So types in the Function name? >> + void set(const char *value); >> + void set(const std::string &value); >> + >> + bool getBool() const; >> + int getInt() const; >> + int getInt64() const; > > This should return a 64-bit integer. Indeed. > >> + std::string getString() const; >> + >> + std::string toString() const; >> + >> +private: >> + ValueType type_; >> + >> + union { >> + bool bool_; >> + int integer_; >> + int64_t integer64_; >> + }; >> + std::string string_; > > As you've noticed, usage of an std::string makes it impossible to make > the string_ member part of the value union, which reduces memory and CPU > efficiency. Every integer value will need to construct an std::string. > For this reason I would store the data for complex types as a pointer > instead, and allocate it dynamically. It could be a pointer to a > std::string, or just a const char *. The latter would likely be more > efficient. Indeed. I think this is looking to get dropped rather than converted at the moment. I wanted to make the Value class a generic re-usable class that could be re-used in the existing Option parsers, and V4L2Controls, because we are already repeating the same patterns across the code base. But Jacopo doesn't want to make use of it there either ... >> +}; >> + >> +std::ostream &operator<<(std::ostream &stream, const Value &value); >> + >> +} /* namespace libcamera */ >> + >> +#endif /* __LIBCAMERA_VALUE_H__ */ >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build >> index 68c7ce14b5b4..8e68373118df 100644 >> --- a/src/libcamera/meson.build >> +++ b/src/libcamera/meson.build >> @@ -27,6 +27,7 @@ libcamera_sources = files([ >> 'v4l2_device.cpp', >> 'v4l2_subdevice.cpp', >> 'v4l2_videodevice.cpp', >> + 'value.cpp', >> 'version.cpp', >> ]) >> >> diff --git a/src/libcamera/value.cpp b/src/libcamera/value.cpp >> new file mode 100644 >> index 000000000000..f713907b38dd >> --- /dev/null >> +++ b/src/libcamera/value.cpp >> @@ -0,0 +1,226 @@ >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> +/* >> + * Copyright (C) 2019, Google Inc. >> + * >> + * value.cpp - Abstract value handling >> + */ >> + >> +#include <limits.h> >> +#include <sstream> >> +#include <string> >> + >> +#include <libcamera/value.h> > > Please move this as the first include to ensure that it compiles without > depending on any implicitly included headers. Done. > >> + >> +#include "log.h" // For ASSERT() > > You don't need to keep the comment. Removed. > >> + >> +/** >> + * \file value.h >> + * \brief Handles variant value types as an abstract object >> + */ >> + >> +namespace libcamera { >> + >> +/** >> + * \enum ValueType >> + * Determines the type of value represented by a \a Value >> + * \var ValueNull >> + * Identifies an unset value >> + * \var ValueBool >> + * Identifies controls storing a boolean value >> + * \var ValueInteger >> + * Identifies controls storing an integer value >> + * \var ValueString >> + * Identifies controls storing a string value > > You're missing the 64-bit integer. > Added. >> + */ >> + >> +/** >> + * \class Value >> + * \brief Abstract a value for a common data exchange >> + */ >> + >> +/** >> + * \brief Construct an empty Value. >> + * The Value must be \a set() before being used. >> + */ >> +Value::Value() >> + : type_(ValueNull) >> +{ >> +} >> + >> +/** >> + * \brief Construct a Boolean Value >> + * \param[in] value Boolean value to store >> + */ >> +Value::Value(bool value) >> + : type_(ValueBool), bool_(value) >> +{ >> +} >> + >> +/** >> + * \brief Construct an integer Value >> + * \param[in] value Integer value to store >> + */ >> +Value::Value(int value) >> + : type_(ValueInteger), integer_(value) >> +{ >> +} >> + >> +/** >> + * \brief Construct a string Value >> + * \param[in] value String representation to store >> + */ >> +Value::Value(const char *value) >> + : type_(ValueString), string_(value) >> +{ >> +} >> + >> +/** >> + * \brief Construct a string Value >> + * \param[in] value String representation to store >> + */ >> +Value::Value(const std::string &value) >> + : type_(ValueString), string_(value) >> +{ >> +} >> + >> +/** >> + * \fn Value::type >> + * \brief Return the type of value represented by the object >> + */ >> + >> +/** >> + * \fn Value::isNull >> + * \brief Determines if the Value is initialised > > s/Determines/Determine/ > >> + * \return true if the value type is ValueNull, false otherwise > > s/true/True/ > >> + */ >> + >> +/** >> + * \brief Set the value with a boolean >> + * \param[in] value Boolean value to store >> + */ >> +void Value::set(bool value) >> +{ >> + type_ = ValueBool; >> + bool_ = value; >> +} >> + >> +/** >> + * \brief Set the value with an integer >> + * \param[in] value Integer value to store >> + */ >> +void Value::set(int value) >> +{ >> + type_ = ValueInteger; >> + integer_ = value; >> +} >> + >> +/** >> + * \brief Set the value with a 64 bit integer >> + * \param[in] value Integer value to store >> + */ >> +void Value::set(int64_t value) >> +{ >> + type_ = ValueInteger64; >> + integer64_ = value; >> +} >> + >> +/** >> + * \brief Set the value with a string representation >> + * \param[in] value String value to store >> + */ >> +void Value::set(const char *value) >> +{ >> + type_ = ValueString; >> + string_ = value; >> +} >> + >> +/** >> + * \brief Set the value with a string representation >> + * \param[in] value String value to store >> + */ >> +void Value::set(const std::string &value) >> +{ >> + type_ = ValueString; >> + string_ = value; >> +} >> + >> +/** >> + * \brief Get the boolean value. >> + * >> + * The Value type must be Boolean. >> + */ >> +bool Value::getBool() const >> +{ >> + ASSERT(type_ == ValueBool); > > That's quite dangerous... Isn't there a large risk that the value will > be set to a type and retrieved using a different but compatible type, > and that the issue would only be caught at runtime, possibly in uncommon > cases ? For instance consider the following code That's why I put the assert in - to catch those issues early? Warnings and Error prints will scroll off the logs... > > Value v; > > if (likely(condition)) > value.set(3); > else > value.set(0x100000000ULL); > > ... > > int i = v.getInt(); > > This will cause a crash in the unlikely event that condition is false, > and could get unnoticed through testing. An explicit set API with type > names may help there. Given that the Value class will be used for > controls, I think we'll have to make sure we don't crash, invalid types > will need to be caught and reported. We could also decide than an > invalid type would result in the control taking a default value instead > of crashing (but then this should be logged). I'm not sure taking a default value is appropriate either. I don't think any 'default' would be correct. IMO - the types were important, and the asserts were to ensure strict typing when used. If the set() call is changed to setInt() would this make you more comfortable? > >> + >> + return bool_; >> +} >> + >> +/** >> + * \brief Get the integer value. >> + * >> + * The Value type must be Integer. >> + */ >> +int Value::getInt() const >> +{ >> + ASSERT(type_ == ValueInteger); >> + >> + return integer_; >> +} >> + >> +/** >> + * \brief Get the 64bit integer value. >> + * >> + * The Value type must be Integer64. >> + */ >> +int Value::getInt64() const >> +{ >> + ASSERT(type_ == ValueInteger64); >> + >> + return integer64_; >> +} >> + >> +/** >> + * \brief Get the string value. >> + * >> + * The Value type must be String. >> + */ >> +std::string Value::getString() const >> +{ >> + ASSERT(type_ == ValueString); >> + >> + return string_; >> +} >> + >> +/** >> + * \brief Prepare a string representation of the Value >> + */ >> +std::string Value::toString() const >> +{ >> + switch (type_) { >> + case ValueNull: >> + return "<NULL>"; >> + case ValueBool: >> + return bool_ ? "True" : "False"; >> + case ValueInteger: >> + return std::to_string(integer_); >> + case ValueInteger64: >> + return std::to_string(integer64_); >> + case ValueString: >> + return string_; >> + } >> + >> + /* Unreachable */ >> + return "<ValueType Error>"; > > Do you need this ? Does the compiler complain when the switch-case > handles all the cases ? > Yup. ../src/libcamera/value.cpp: In member function ‘std::__cxx11::string libcamera::Value::toString() const’: ../src/libcamera/value.cpp:212:1: error: control reaches end of non-void function [-Werror=return-type] } ^ cc1plus: all warnings being treated as errors I can move it to a 'default:' statement which then removes the warning - but should still be redundant/ unreachable. >> +} >> + >> +/** >> + * \brief Provide a string stream representation of the Value \a value to >> + * the \a stream. > > It's not a string stream, it's an output stream (and you should include > ostream instead of sstream). > >> + */ >> +std::ostream &operator<<(std::ostream &stream, const Value &value) >> +{ >> + return stream << value.toString(); >> +} >> + >> +} /* namespace libcamera */ >
Hi Kieran, On Mon, Jun 24, 2019 at 10:09:06AM +0100, Kieran Bingham wrote: > On 22/06/2019 23:39, Laurent Pinchart wrote: > > On Fri, Jun 21, 2019 at 05:13:53PM +0100, Kieran Bingham wrote: > >> To facilitate passing typed data generically, implement a class which stores > >> the type and value as efficiently as possible. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> include/libcamera/meson.build | 1 + > >> include/libcamera/value.h | 63 ++++++++++ > >> src/libcamera/meson.build | 1 + > >> src/libcamera/value.cpp | 226 ++++++++++++++++++++++++++++++++++ > >> 4 files changed, 291 insertions(+) > >> create mode 100644 include/libcamera/value.h > >> create mode 100644 src/libcamera/value.cpp > >> > >> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > >> index 201832105457..eb2211ae1fc3 100644 > >> --- a/include/libcamera/meson.build > >> +++ b/include/libcamera/meson.build > >> @@ -12,6 +12,7 @@ libcamera_api = files([ > >> 'signal.h', > >> 'stream.h', > >> 'timer.h', > >> + 'value.h', > >> 'version.h', > >> ]) > >> > >> diff --git a/include/libcamera/value.h b/include/libcamera/value.h > >> new file mode 100644 > >> index 000000000000..00c5d53d5cc0 > >> --- /dev/null > >> +++ b/include/libcamera/value.h > >> @@ -0,0 +1,63 @@ > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > >> +/* > >> + * Copyright (C) 2019, Google Inc. > >> + * > >> + * value.h - Abstract value handling > >> + */ > >> + > >> +#include <map> > >> + > >> +#ifndef __LIBCAMERA_VALUE_H__ > >> +#define __LIBCAMERA_VALUE_H__ > >> + > >> +namespace libcamera { > >> + > >> +enum ValueType : uint8_t { > > > > Do we need an explicit uint8_t here ? It won't save memory as the next > > member of the Value class will be aligned to a 64-bits boundary. > > True, but ValueType might be used in other locations too? Although then > they'd only be used for checking the type - so storage isn't really an > issue there either. > > My actual reason for playing around with specifying the types on enums > was to explore the effects on the ABI. > > The ValueType enum should certainly never exceed 255 entries, which is > one benefit of being explicitly smaller - but I do think on this > occasion it is completely redundant so I'll just remove it now. > > >> + ValueNull, > > > > Let's call it ValueUnset or ValueNone. Null has a well established > > meaning. > > I started with ValueNone ... but moved to ValueNull ... but I'm happy to > go back again :-) > > >> + ValueBool, > >> + ValueInteger, > >> + ValueInteger64, > >> + ValueString, > >> +}; > >> + > >> +class Value > > > > I think I would call this ControlValue for now, and then expand it to a > > more generic solution if needed, instead of trying to be too generic to > > start with. > > And this whole class came from ControlValue > > >> +{ > >> +public: > >> + Value(); > >> + Value(bool value); > >> + Value(int value); > >> + Value(const char *value); > >> + Value(const std::string &value); > >> + > >> + ValueType type() const { return type_; }; > >> + bool isNull() const { return type_ == ValueNull; }; > > > > isUnset() ? > > Well if it's not Null anymore, shouldn't it be isNone? > > Or do you prefer (Un)'set' to mirror the set() call. I'm fine with either, let's just avoid "null" and make the type name and function name consistent. > >> + > >> + void set(bool value); > >> + void set(int value); > >> + void set(int64_t value); > > > > This is asking for trouble, you will often end up with the wrong integer > > type without even noticing, unless you can remember and apply flawlessly > > the C++ integer types implicit conversions rules. > > Hrm... So types in the Function name? There are multiple options. One of them is to treat all integer types as an int64_t internally, and have getInt() and getInt64() operate properly on them. Another option would be to use explicit function names. I like the former in theory, but it requires conversions in the getters, so it may be a little bit less efficient. Simplicity of use or efficiency in this case ? :-) > >> + void set(const char *value); > >> + void set(const std::string &value); > >> + > >> + bool getBool() const; > >> + int getInt() const; > >> + int getInt64() const; > > > > This should return a 64-bit integer. > > Indeed. > > >> + std::string getString() const; > >> + > >> + std::string toString() const; > >> + > >> +private: > >> + ValueType type_; > >> + > >> + union { > >> + bool bool_; > >> + int integer_; > >> + int64_t integer64_; > >> + }; > >> + std::string string_; > > > > As you've noticed, usage of an std::string makes it impossible to make > > the string_ member part of the value union, which reduces memory and CPU > > efficiency. Every integer value will need to construct an std::string. > > For this reason I would store the data for complex types as a pointer > > instead, and allocate it dynamically. It could be a pointer to a > > std::string, or just a const char *. The latter would likely be more > > efficient. > > Indeed. I think this is looking to get dropped rather than converted at > the moment. > > I wanted to make the Value class a generic re-usable class that could be > re-used in the existing Option parsers, and V4L2Controls, because we are > already repeating the same patterns across the code base. > > But Jacopo doesn't want to make use of it there either ... Let's model it to be efficient for its main use cases, and then we can extend it to cover additional use cases if possible without an efficiency impact. I quite like the idea of using it for both libcamera and V4L2 controls though, but that could be done later (and isn't a strong requirement). > >> +}; > >> + > >> +std::ostream &operator<<(std::ostream &stream, const Value &value); > >> + > >> +} /* namespace libcamera */ > >> + > >> +#endif /* __LIBCAMERA_VALUE_H__ */ > >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > >> index 68c7ce14b5b4..8e68373118df 100644 > >> --- a/src/libcamera/meson.build > >> +++ b/src/libcamera/meson.build > >> @@ -27,6 +27,7 @@ libcamera_sources = files([ > >> 'v4l2_device.cpp', > >> 'v4l2_subdevice.cpp', > >> 'v4l2_videodevice.cpp', > >> + 'value.cpp', > >> 'version.cpp', > >> ]) > >> > >> diff --git a/src/libcamera/value.cpp b/src/libcamera/value.cpp > >> new file mode 100644 > >> index 000000000000..f713907b38dd > >> --- /dev/null > >> +++ b/src/libcamera/value.cpp > >> @@ -0,0 +1,226 @@ > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > >> +/* > >> + * Copyright (C) 2019, Google Inc. > >> + * > >> + * value.cpp - Abstract value handling > >> + */ > >> + > >> +#include <limits.h> > >> +#include <sstream> > >> +#include <string> > >> + > >> +#include <libcamera/value.h> > > > > Please move this as the first include to ensure that it compiles without > > depending on any implicitly included headers. > > Done. > > >> + > >> +#include "log.h" // For ASSERT() > > > > You don't need to keep the comment. > > Removed. > > >> + > >> +/** > >> + * \file value.h > >> + * \brief Handles variant value types as an abstract object > >> + */ > >> + > >> +namespace libcamera { > >> + > >> +/** > >> + * \enum ValueType > >> + * Determines the type of value represented by a \a Value > >> + * \var ValueNull > >> + * Identifies an unset value > >> + * \var ValueBool > >> + * Identifies controls storing a boolean value > >> + * \var ValueInteger > >> + * Identifies controls storing an integer value > >> + * \var ValueString > >> + * Identifies controls storing a string value > > > > You're missing the 64-bit integer. > > Added. > > >> + */ > >> + > >> +/** > >> + * \class Value > >> + * \brief Abstract a value for a common data exchange > >> + */ > >> + > >> +/** > >> + * \brief Construct an empty Value. > >> + * The Value must be \a set() before being used. > >> + */ > >> +Value::Value() > >> + : type_(ValueNull) > >> +{ > >> +} > >> + > >> +/** > >> + * \brief Construct a Boolean Value > >> + * \param[in] value Boolean value to store > >> + */ > >> +Value::Value(bool value) > >> + : type_(ValueBool), bool_(value) > >> +{ > >> +} > >> + > >> +/** > >> + * \brief Construct an integer Value > >> + * \param[in] value Integer value to store > >> + */ > >> +Value::Value(int value) > >> + : type_(ValueInteger), integer_(value) > >> +{ > >> +} > >> + > >> +/** > >> + * \brief Construct a string Value > >> + * \param[in] value String representation to store > >> + */ > >> +Value::Value(const char *value) > >> + : type_(ValueString), string_(value) > >> +{ > >> +} > >> + > >> +/** > >> + * \brief Construct a string Value > >> + * \param[in] value String representation to store > >> + */ > >> +Value::Value(const std::string &value) > >> + : type_(ValueString), string_(value) > >> +{ > >> +} > >> + > >> +/** > >> + * \fn Value::type > >> + * \brief Return the type of value represented by the object > >> + */ > >> + > >> +/** > >> + * \fn Value::isNull > >> + * \brief Determines if the Value is initialised > > > > s/Determines/Determine/ > > > >> + * \return true if the value type is ValueNull, false otherwise > > > > s/true/True/ > > > >> + */ > >> + > >> +/** > >> + * \brief Set the value with a boolean > >> + * \param[in] value Boolean value to store > >> + */ > >> +void Value::set(bool value) > >> +{ > >> + type_ = ValueBool; > >> + bool_ = value; > >> +} > >> + > >> +/** > >> + * \brief Set the value with an integer > >> + * \param[in] value Integer value to store > >> + */ > >> +void Value::set(int value) > >> +{ > >> + type_ = ValueInteger; > >> + integer_ = value; > >> +} > >> + > >> +/** > >> + * \brief Set the value with a 64 bit integer > >> + * \param[in] value Integer value to store > >> + */ > >> +void Value::set(int64_t value) > >> +{ > >> + type_ = ValueInteger64; > >> + integer64_ = value; > >> +} > >> + > >> +/** > >> + * \brief Set the value with a string representation > >> + * \param[in] value String value to store > >> + */ > >> +void Value::set(const char *value) > >> +{ > >> + type_ = ValueString; > >> + string_ = value; > >> +} > >> + > >> +/** > >> + * \brief Set the value with a string representation > >> + * \param[in] value String value to store > >> + */ > >> +void Value::set(const std::string &value) > >> +{ > >> + type_ = ValueString; > >> + string_ = value; > >> +} > >> + > >> +/** > >> + * \brief Get the boolean value. > >> + * > >> + * The Value type must be Boolean. > >> + */ > >> +bool Value::getBool() const > >> +{ > >> + ASSERT(type_ == ValueBool); > > > > That's quite dangerous... Isn't there a large risk that the value will > > be set to a type and retrieved using a different but compatible type, > > and that the issue would only be caught at runtime, possibly in uncommon > > cases ? For instance consider the following code > > That's why I put the assert in - to catch those issues early? > > Warnings and Error prints will scroll off the logs... With the condition being unlikely the ASSERT() may only trigger in production, which wouldn't be very nice. > > > > Value v; > > > > if (likely(condition)) > > value.set(3); > > else > > value.set(0x100000000ULL); > > > > ... > > > > int i = v.getInt(); > > > > This will cause a crash in the unlikely event that condition is false, > > and could get unnoticed through testing. An explicit set API with type > > names may help there. Given that the Value class will be used for > > controls, I think we'll have to make sure we don't crash, invalid types > > will need to be caught and reported. We could also decide than an > > invalid type would result in the control taking a default value instead > > of crashing (but then this should be logged). > > I'm not sure taking a default value is appropriate either. I don't think > any 'default' would be correct. > > IMO - the types were important, and the asserts were to ensure strict > typing when used. Could we do it the Qt way, where the get functions return an invalid value if the type isn't convertible ? For integers and booleans there's no invalid values, so Qt adds an optional bool *ok argument. In practice I expect the callers to rarely use it though, so I think we need to pick between two error handling behaviours: returning a "default" value (0 for integers for instance) or crashing. What's best ? :-) Now that I wrote this, I realised that ASSERT() will be compiled out in release builds. Having it in debug builds is thus likely a good idea, but we still need to decide on how to handle release builds. > If the set() call is changed to setInt() would this make you more > comfortable? A little bit :-) > >> + > >> + return bool_; > >> +} > >> + > >> +/** > >> + * \brief Get the integer value. > >> + * > >> + * The Value type must be Integer. > >> + */ > >> +int Value::getInt() const > >> +{ > >> + ASSERT(type_ == ValueInteger); > >> + > >> + return integer_; > >> +} > >> + > >> +/** > >> + * \brief Get the 64bit integer value. > >> + * > >> + * The Value type must be Integer64. > >> + */ > >> +int Value::getInt64() const > >> +{ > >> + ASSERT(type_ == ValueInteger64); > >> + > >> + return integer64_; > >> +} > >> + > >> +/** > >> + * \brief Get the string value. > >> + * > >> + * The Value type must be String. > >> + */ > >> +std::string Value::getString() const > >> +{ > >> + ASSERT(type_ == ValueString); > >> + > >> + return string_; > >> +} > >> + > >> +/** > >> + * \brief Prepare a string representation of the Value > >> + */ > >> +std::string Value::toString() const > >> +{ > >> + switch (type_) { > >> + case ValueNull: > >> + return "<NULL>"; > >> + case ValueBool: > >> + return bool_ ? "True" : "False"; > >> + case ValueInteger: > >> + return std::to_string(integer_); > >> + case ValueInteger64: > >> + return std::to_string(integer64_); > >> + case ValueString: > >> + return string_; > >> + } > >> + > >> + /* Unreachable */ > >> + return "<ValueType Error>"; > > > > Do you need this ? Does the compiler complain when the switch-case > > handles all the cases ? > > Yup. > > ../src/libcamera/value.cpp: In member function ‘std::__cxx11::string > libcamera::Value::toString() const’: > ../src/libcamera/value.cpp:212:1: error: control reaches end of non-void > function [-Werror=return-type] > } > ^ > cc1plus: all warnings being treated as errors > > I can move it to a 'default:' statement which then removes the warning - > but should still be redundant/ unreachable. A default statement sounds nice, but I think that an error after the switch() is better as in that case the compiler should warn you if you add a new enum entry without adding a corresponding case. > >> +} > >> + > >> +/** > >> + * \brief Provide a string stream representation of the Value \a value to > >> + * the \a stream. > > > > It's not a string stream, it's an output stream (and you should include > > ostream instead of sstream). > > > >> + */ > >> +std::ostream &operator<<(std::ostream &stream, const Value &value) > >> +{ > >> + return stream << value.toString(); > >> +} > >> + > >> +} /* namespace libcamera */
Hi Laurent, On 24/06/2019 13:09, Laurent Pinchart wrote: > Hi Kieran, > > On Mon, Jun 24, 2019 at 10:09:06AM +0100, Kieran Bingham wrote: >> On 22/06/2019 23:39, Laurent Pinchart wrote: >>> On Fri, Jun 21, 2019 at 05:13:53PM +0100, Kieran Bingham wrote: >>>> To facilitate passing typed data generically, implement a class which stores >>>> the type and value as efficiently as possible. >>>> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> --- >>>> include/libcamera/meson.build | 1 + >>>> include/libcamera/value.h | 63 ++++++++++ >>>> src/libcamera/meson.build | 1 + >>>> src/libcamera/value.cpp | 226 ++++++++++++++++++++++++++++++++++ >>>> 4 files changed, 291 insertions(+) >>>> create mode 100644 include/libcamera/value.h >>>> create mode 100644 src/libcamera/value.cpp >>>> >>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build >>>> index 201832105457..eb2211ae1fc3 100644 >>>> --- a/include/libcamera/meson.build >>>> +++ b/include/libcamera/meson.build >>>> @@ -12,6 +12,7 @@ libcamera_api = files([ >>>> 'signal.h', >>>> 'stream.h', >>>> 'timer.h', >>>> + 'value.h', >>>> 'version.h', >>>> ]) >>>> >>>> diff --git a/include/libcamera/value.h b/include/libcamera/value.h >>>> new file mode 100644 >>>> index 000000000000..00c5d53d5cc0 >>>> --- /dev/null >>>> +++ b/include/libcamera/value.h >>>> @@ -0,0 +1,63 @@ >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >>>> +/* >>>> + * Copyright (C) 2019, Google Inc. >>>> + * >>>> + * value.h - Abstract value handling >>>> + */ >>>> + >>>> +#include <map> >>>> + >>>> +#ifndef __LIBCAMERA_VALUE_H__ >>>> +#define __LIBCAMERA_VALUE_H__ >>>> + >>>> +namespace libcamera { >>>> + >>>> +enum ValueType : uint8_t { >>> >>> Do we need an explicit uint8_t here ? It won't save memory as the next >>> member of the Value class will be aligned to a 64-bits boundary. >> >> True, but ValueType might be used in other locations too? Although then >> they'd only be used for checking the type - so storage isn't really an >> issue there either. >> >> My actual reason for playing around with specifying the types on enums >> was to explore the effects on the ABI. >> >> The ValueType enum should certainly never exceed 255 entries, which is >> one benefit of being explicitly smaller - but I do think on this >> occasion it is completely redundant so I'll just remove it now. >> >>>> + ValueNull, >>> >>> Let's call it ValueUnset or ValueNone. Null has a well established >>> meaning. >> >> I started with ValueNone ... but moved to ValueNull ... but I'm happy to >> go back again :-) >> >>>> + ValueBool, >>>> + ValueInteger, >>>> + ValueInteger64, >>>> + ValueString, >>>> +}; >>>> + >>>> +class Value >>> >>> I think I would call this ControlValue for now, and then expand it to a >>> more generic solution if needed, instead of trying to be too generic to >>> start with. >> >> And this whole class came from ControlValue >> >>>> +{ >>>> +public: >>>> + Value(); >>>> + Value(bool value); >>>> + Value(int value); >>>> + Value(const char *value); >>>> + Value(const std::string &value); >>>> + >>>> + ValueType type() const { return type_; }; >>>> + bool isNull() const { return type_ == ValueNull; }; >>> >>> isUnset() ? >> >> Well if it's not Null anymore, shouldn't it be isNone? >> >> Or do you prefer (Un)'set' to mirror the set() call. > > I'm fine with either, let's just avoid "null" and make the type name and > function name consistent. > >>>> + >>>> + void set(bool value); >>>> + void set(int value); >>>> + void set(int64_t value); >>> >>> This is asking for trouble, you will often end up with the wrong integer >>> type without even noticing, unless you can remember and apply flawlessly >>> the C++ integer types implicit conversions rules. >> >> Hrm... So types in the Function name? > > There are multiple options. One of them is to treat all integer types as > an int64_t internally, and have getInt() and getInt64() operate properly > on them. Another option would be to use explicit function names. I like > the former in theory, but it requires conversions in the getters, so it > may be a little bit less efficient. Simplicity of use or efficiency in > this case ? :-) > >>>> + void set(const char *value); >>>> + void set(const std::string &value); >>>> + >>>> + bool getBool() const; >>>> + int getInt() const; >>>> + int getInt64() const; >>> >>> This should return a 64-bit integer. >> >> Indeed. >> >>>> + std::string getString() const; >>>> + >>>> + std::string toString() const; >>>> + >>>> +private: >>>> + ValueType type_; >>>> + >>>> + union { >>>> + bool bool_; >>>> + int integer_; >>>> + int64_t integer64_; >>>> + }; >>>> + std::string string_; >>> >>> As you've noticed, usage of an std::string makes it impossible to make >>> the string_ member part of the value union, which reduces memory and CPU >>> efficiency. Every integer value will need to construct an std::string. >>> For this reason I would store the data for complex types as a pointer >>> instead, and allocate it dynamically. It could be a pointer to a >>> std::string, or just a const char *. The latter would likely be more >>> efficient. >> >> Indeed. I think this is looking to get dropped rather than converted at >> the moment. >> >> I wanted to make the Value class a generic re-usable class that could be >> re-used in the existing Option parsers, and V4L2Controls, because we are >> already repeating the same patterns across the code base. >> >> But Jacopo doesn't want to make use of it there either ... > > Let's model it to be efficient for its main use cases, and then we can > extend it to cover additional use cases if possible without an > efficiency impact. I quite like the idea of using it for both libcamera > and V4L2 controls though, but that could be done later (and isn't a > strong requirement). Yes, it can be done later, I don't want to postpone Jacopo's series. I'd still like to keep this called Value, do you want it all renamed to ControlValue ? >>>> +}; >>>> + >>>> +std::ostream &operator<<(std::ostream &stream, const Value &value); >>>> + >>>> +} /* namespace libcamera */ >>>> + >>>> +#endif /* __LIBCAMERA_VALUE_H__ */ >>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build >>>> index 68c7ce14b5b4..8e68373118df 100644 >>>> --- a/src/libcamera/meson.build >>>> +++ b/src/libcamera/meson.build >>>> @@ -27,6 +27,7 @@ libcamera_sources = files([ >>>> 'v4l2_device.cpp', >>>> 'v4l2_subdevice.cpp', >>>> 'v4l2_videodevice.cpp', >>>> + 'value.cpp', >>>> 'version.cpp', >>>> ]) >>>> >>>> diff --git a/src/libcamera/value.cpp b/src/libcamera/value.cpp >>>> new file mode 100644 >>>> index 000000000000..f713907b38dd >>>> --- /dev/null >>>> +++ b/src/libcamera/value.cpp >>>> @@ -0,0 +1,226 @@ >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >>>> +/* >>>> + * Copyright (C) 2019, Google Inc. >>>> + * >>>> + * value.cpp - Abstract value handling >>>> + */ >>>> + >>>> +#include <limits.h> >>>> +#include <sstream> >>>> +#include <string> >>>> + >>>> +#include <libcamera/value.h> >>> >>> Please move this as the first include to ensure that it compiles without >>> depending on any implicitly included headers. >> >> Done. >> >>>> + >>>> +#include "log.h" // For ASSERT() >>> >>> You don't need to keep the comment. >> >> Removed. >> >>>> + >>>> +/** >>>> + * \file value.h >>>> + * \brief Handles variant value types as an abstract object >>>> + */ >>>> + >>>> +namespace libcamera { >>>> + >>>> +/** >>>> + * \enum ValueType >>>> + * Determines the type of value represented by a \a Value >>>> + * \var ValueNull >>>> + * Identifies an unset value >>>> + * \var ValueBool >>>> + * Identifies controls storing a boolean value >>>> + * \var ValueInteger >>>> + * Identifies controls storing an integer value >>>> + * \var ValueString >>>> + * Identifies controls storing a string value >>> >>> You're missing the 64-bit integer. >> >> Added. >> >>>> + */ >>>> + >>>> +/** >>>> + * \class Value >>>> + * \brief Abstract a value for a common data exchange >>>> + */ >>>> + >>>> +/** >>>> + * \brief Construct an empty Value. >>>> + * The Value must be \a set() before being used. >>>> + */ >>>> +Value::Value() >>>> + : type_(ValueNull) >>>> +{ >>>> +} >>>> + >>>> +/** >>>> + * \brief Construct a Boolean Value >>>> + * \param[in] value Boolean value to store >>>> + */ >>>> +Value::Value(bool value) >>>> + : type_(ValueBool), bool_(value) >>>> +{ >>>> +} >>>> + >>>> +/** >>>> + * \brief Construct an integer Value >>>> + * \param[in] value Integer value to store >>>> + */ >>>> +Value::Value(int value) >>>> + : type_(ValueInteger), integer_(value) >>>> +{ >>>> +} >>>> + >>>> +/** >>>> + * \brief Construct a string Value >>>> + * \param[in] value String representation to store >>>> + */ >>>> +Value::Value(const char *value) >>>> + : type_(ValueString), string_(value) >>>> +{ >>>> +} >>>> + >>>> +/** >>>> + * \brief Construct a string Value >>>> + * \param[in] value String representation to store >>>> + */ >>>> +Value::Value(const std::string &value) >>>> + : type_(ValueString), string_(value) >>>> +{ >>>> +} >>>> + >>>> +/** >>>> + * \fn Value::type >>>> + * \brief Return the type of value represented by the object >>>> + */ >>>> + >>>> +/** >>>> + * \fn Value::isNull >>>> + * \brief Determines if the Value is initialised >>> >>> s/Determines/Determine/ >>> >>>> + * \return true if the value type is ValueNull, false otherwise >>> >>> s/true/True/ >>> >>>> + */ >>>> + >>>> +/** >>>> + * \brief Set the value with a boolean >>>> + * \param[in] value Boolean value to store >>>> + */ >>>> +void Value::set(bool value) >>>> +{ >>>> + type_ = ValueBool; >>>> + bool_ = value; >>>> +} >>>> + >>>> +/** >>>> + * \brief Set the value with an integer >>>> + * \param[in] value Integer value to store >>>> + */ >>>> +void Value::set(int value) >>>> +{ >>>> + type_ = ValueInteger; >>>> + integer_ = value; >>>> +} >>>> + >>>> +/** >>>> + * \brief Set the value with a 64 bit integer >>>> + * \param[in] value Integer value to store >>>> + */ >>>> +void Value::set(int64_t value) >>>> +{ >>>> + type_ = ValueInteger64; >>>> + integer64_ = value; >>>> +} >>>> + >>>> +/** >>>> + * \brief Set the value with a string representation >>>> + * \param[in] value String value to store >>>> + */ >>>> +void Value::set(const char *value) >>>> +{ >>>> + type_ = ValueString; >>>> + string_ = value; >>>> +} >>>> + >>>> +/** >>>> + * \brief Set the value with a string representation >>>> + * \param[in] value String value to store >>>> + */ >>>> +void Value::set(const std::string &value) >>>> +{ >>>> + type_ = ValueString; >>>> + string_ = value; >>>> +} >>>> + >>>> +/** >>>> + * \brief Get the boolean value. >>>> + * >>>> + * The Value type must be Boolean. >>>> + */ >>>> +bool Value::getBool() const >>>> +{ >>>> + ASSERT(type_ == ValueBool); >>> >>> That's quite dangerous... Isn't there a large risk that the value will >>> be set to a type and retrieved using a different but compatible type, >>> and that the issue would only be caught at runtime, possibly in uncommon >>> cases ? For instance consider the following code >> >> That's why I put the assert in - to catch those issues early? >> >> Warnings and Error prints will scroll off the logs... > > With the condition being unlikely the ASSERT() may only trigger in > production, which wouldn't be very nice. Am I misunderstanding /your/ definition of ASSERT? : > * \def ASSERT(condition) > * \brief Abort program execution if assertion fails > * > * If \a condition is false, ASSERT() logs an error message with the Fatal log > * level and aborts program execution. > * > * If the macro NDEBUG is defined before including log.h, ASSERT() generates no > * code. ASSERT should never fire in production, because NDEBUG should be set for BuildType=Release builds. >>> >>> Value v; >>> >>> if (likely(condition)) >>> value.set(3); >>> else >>> value.set(0x100000000ULL); >>> >>> ... >>> >>> int i = v.getInt(); >>> >>> This will cause a crash in the unlikely event that condition is false, >>> and could get unnoticed through testing. An explicit set API with type >>> names may help there. Given that the Value class will be used for >>> controls, I think we'll have to make sure we don't crash, invalid types >>> will need to be caught and reported. We could also decide than an >>> invalid type would result in the control taking a default value instead >>> of crashing (but then this should be logged). >> >> I'm not sure taking a default value is appropriate either. I don't think >> any 'default' would be correct. >> >> IMO - the types were important, and the asserts were to ensure strict >> typing when used. > > Could we do it the Qt way, where the get functions return an invalid > value if the type isn't convertible ? For integers and booleans there's > no invalid values, so Qt adds an optional bool *ok argument. In practice Eurgh ... an 'ok' bool sounds icky... I'll look at the Qt code ... > I expect the callers to rarely use it though, so I think we need to pick > between two error handling behaviours: returning a "default" value (0 > for integers for instance) or crashing. What's best ? :-) Crashing in debug builds only :D and returning an undefined value otherwise. (it will be whatever the contents of the union is, with whatever implicit type conversion occurs, which 'may' be as correct as it can be). > > Now that I wrote this, I realised that ASSERT() will be compiled out in > release builds. Having it in debug builds is thus likely a good idea, > but we still need to decide on how to handle release builds. Aha, well I didn't need to paste that in above then :) Too late ! I'll leave it there :P > >> If the set() call is changed to setInt() would this make you more >> comfortable? > > A little bit :-) Ok - I'll remove the implicit type sets. >>>> + >>>> + return bool_; >>>> +} >>>> + >>>> +/** >>>> + * \brief Get the integer value. >>>> + * >>>> + * The Value type must be Integer. >>>> + */ >>>> +int Value::getInt() const >>>> +{ >>>> + ASSERT(type_ == ValueInteger); >>>> + >>>> + return integer_; >>>> +} >>>> + >>>> +/** >>>> + * \brief Get the 64bit integer value. >>>> + * >>>> + * The Value type must be Integer64. >>>> + */ >>>> +int Value::getInt64() const >>>> +{ >>>> + ASSERT(type_ == ValueInteger64); >>>> + >>>> + return integer64_; >>>> +} >>>> + >>>> +/** >>>> + * \brief Get the string value. >>>> + * >>>> + * The Value type must be String. >>>> + */ >>>> +std::string Value::getString() const >>>> +{ >>>> + ASSERT(type_ == ValueString); >>>> + >>>> + return string_; >>>> +} >>>> + >>>> +/** >>>> + * \brief Prepare a string representation of the Value >>>> + */ >>>> +std::string Value::toString() const >>>> +{ >>>> + switch (type_) { >>>> + case ValueNull: >>>> + return "<NULL>"; >>>> + case ValueBool: >>>> + return bool_ ? "True" : "False"; >>>> + case ValueInteger: >>>> + return std::to_string(integer_); >>>> + case ValueInteger64: >>>> + return std::to_string(integer64_); >>>> + case ValueString: >>>> + return string_; >>>> + } >>>> + >>>> + /* Unreachable */ >>>> + return "<ValueType Error>"; >>> >>> Do you need this ? Does the compiler complain when the switch-case >>> handles all the cases ? >> >> Yup. >> >> ../src/libcamera/value.cpp: In member function ‘std::__cxx11::string >> libcamera::Value::toString() const’: >> ../src/libcamera/value.cpp:212:1: error: control reaches end of non-void >> function [-Werror=return-type] >> } >> ^ >> cc1plus: all warnings being treated as errors >> >> I can move it to a 'default:' statement which then removes the warning - >> but should still be redundant/ unreachable. > > A default statement sounds nice, but I think that an error after the > switch() is better as in that case the compiler should warn you if you > add a new enum entry without adding a corresponding case. I agree, I'd rather the compiler tell us if we have a missing enum type. > >>>> +} >>>> + >>>> +/** >>>> + * \brief Provide a string stream representation of the Value \a value to >>>> + * the \a stream. >>> >>> It's not a string stream, it's an output stream (and you should include >>> ostream instead of sstream). >>> >>>> + */ >>>> +std::ostream &operator<<(std::ostream &stream, const Value &value) >>>> +{ >>>> + return stream << value.toString(); >>>> +} >>>> + >>>> +} /* namespace libcamera */ >
Hi Kieran, On Mon, Jun 24, 2019 at 04:39:41PM +0100, Kieran Bingham wrote: > On 24/06/2019 13:09, Laurent Pinchart wrote: > > On Mon, Jun 24, 2019 at 10:09:06AM +0100, Kieran Bingham wrote: > >> On 22/06/2019 23:39, Laurent Pinchart wrote: > >>> On Fri, Jun 21, 2019 at 05:13:53PM +0100, Kieran Bingham wrote: > >>>> To facilitate passing typed data generically, implement a class which stores > >>>> the type and value as efficiently as possible. > >>>> > >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>> --- > >>>> include/libcamera/meson.build | 1 + > >>>> include/libcamera/value.h | 63 ++++++++++ > >>>> src/libcamera/meson.build | 1 + > >>>> src/libcamera/value.cpp | 226 ++++++++++++++++++++++++++++++++++ > >>>> 4 files changed, 291 insertions(+) > >>>> create mode 100644 include/libcamera/value.h > >>>> create mode 100644 src/libcamera/value.cpp > >>>> > >>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > >>>> index 201832105457..eb2211ae1fc3 100644 > >>>> --- a/include/libcamera/meson.build > >>>> +++ b/include/libcamera/meson.build > >>>> @@ -12,6 +12,7 @@ libcamera_api = files([ > >>>> 'signal.h', > >>>> 'stream.h', > >>>> 'timer.h', > >>>> + 'value.h', > >>>> 'version.h', > >>>> ]) > >>>> > >>>> diff --git a/include/libcamera/value.h b/include/libcamera/value.h > >>>> new file mode 100644 > >>>> index 000000000000..00c5d53d5cc0 > >>>> --- /dev/null > >>>> +++ b/include/libcamera/value.h > >>>> @@ -0,0 +1,63 @@ > >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > >>>> +/* > >>>> + * Copyright (C) 2019, Google Inc. > >>>> + * > >>>> + * value.h - Abstract value handling > >>>> + */ > >>>> + > >>>> +#include <map> > >>>> + > >>>> +#ifndef __LIBCAMERA_VALUE_H__ > >>>> +#define __LIBCAMERA_VALUE_H__ > >>>> + > >>>> +namespace libcamera { > >>>> + > >>>> +enum ValueType : uint8_t { > >>> > >>> Do we need an explicit uint8_t here ? It won't save memory as the next > >>> member of the Value class will be aligned to a 64-bits boundary. > >> > >> True, but ValueType might be used in other locations too? Although then > >> they'd only be used for checking the type - so storage isn't really an > >> issue there either. > >> > >> My actual reason for playing around with specifying the types on enums > >> was to explore the effects on the ABI. > >> > >> The ValueType enum should certainly never exceed 255 entries, which is > >> one benefit of being explicitly smaller - but I do think on this > >> occasion it is completely redundant so I'll just remove it now. > >> > >>>> + ValueNull, > >>> > >>> Let's call it ValueUnset or ValueNone. Null has a well established > >>> meaning. > >> > >> I started with ValueNone ... but moved to ValueNull ... but I'm happy to > >> go back again :-) > >> > >>>> + ValueBool, > >>>> + ValueInteger, > >>>> + ValueInteger64, > >>>> + ValueString, > >>>> +}; > >>>> + > >>>> +class Value > >>> > >>> I think I would call this ControlValue for now, and then expand it to a > >>> more generic solution if needed, instead of trying to be too generic to > >>> start with. > >> > >> And this whole class came from ControlValue > >> > >>>> +{ > >>>> +public: > >>>> + Value(); > >>>> + Value(bool value); > >>>> + Value(int value); > >>>> + Value(const char *value); > >>>> + Value(const std::string &value); > >>>> + > >>>> + ValueType type() const { return type_; }; > >>>> + bool isNull() const { return type_ == ValueNull; }; > >>> > >>> isUnset() ? > >> > >> Well if it's not Null anymore, shouldn't it be isNone? > >> > >> Or do you prefer (Un)'set' to mirror the set() call. > > > > I'm fine with either, let's just avoid "null" and make the type name and > > function name consistent. > > > >>>> + > >>>> + void set(bool value); > >>>> + void set(int value); > >>>> + void set(int64_t value); > >>> > >>> This is asking for trouble, you will often end up with the wrong integer > >>> type without even noticing, unless you can remember and apply flawlessly > >>> the C++ integer types implicit conversions rules. > >> > >> Hrm... So types in the Function name? > > > > There are multiple options. One of them is to treat all integer types as > > an int64_t internally, and have getInt() and getInt64() operate properly > > on them. Another option would be to use explicit function names. I like > > the former in theory, but it requires conversions in the getters, so it > > may be a little bit less efficient. Simplicity of use or efficiency in > > this case ? :-) > > > >>>> + void set(const char *value); > >>>> + void set(const std::string &value); > >>>> + > >>>> + bool getBool() const; > >>>> + int getInt() const; > >>>> + int getInt64() const; > >>> > >>> This should return a 64-bit integer. > >> > >> Indeed. > >> > >>>> + std::string getString() const; > >>>> + > >>>> + std::string toString() const; > >>>> + > >>>> +private: > >>>> + ValueType type_; > >>>> + > >>>> + union { > >>>> + bool bool_; > >>>> + int integer_; > >>>> + int64_t integer64_; > >>>> + }; > >>>> + std::string string_; > >>> > >>> As you've noticed, usage of an std::string makes it impossible to make > >>> the string_ member part of the value union, which reduces memory and CPU > >>> efficiency. Every integer value will need to construct an std::string. > >>> For this reason I would store the data for complex types as a pointer > >>> instead, and allocate it dynamically. It could be a pointer to a > >>> std::string, or just a const char *. The latter would likely be more > >>> efficient. > >> > >> Indeed. I think this is looking to get dropped rather than converted at > >> the moment. > >> > >> I wanted to make the Value class a generic re-usable class that could be > >> re-used in the existing Option parsers, and V4L2Controls, because we are > >> already repeating the same patterns across the code base. > >> > >> But Jacopo doesn't want to make use of it there either ... > > > > Let's model it to be efficient for its main use cases, and then we can > > extend it to cover additional use cases if possible without an > > efficiency impact. I quite like the idea of using it for both libcamera > > and V4L2 controls though, but that could be done later (and isn't a > > strong requirement). > > Yes, it can be done later, I don't want to postpone Jacopo's series. > > I'd still like to keep this called Value, do you want it all renamed to > ControlValue ? I have a slight preference for that, especially given that the implementation is restricted to the types needed for controls, and that it tries to optimise for control-related use cases. > >>>> +}; > >>>> + > >>>> +std::ostream &operator<<(std::ostream &stream, const Value &value); > >>>> + > >>>> +} /* namespace libcamera */ > >>>> + > >>>> +#endif /* __LIBCAMERA_VALUE_H__ */ > >>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > >>>> index 68c7ce14b5b4..8e68373118df 100644 > >>>> --- a/src/libcamera/meson.build > >>>> +++ b/src/libcamera/meson.build > >>>> @@ -27,6 +27,7 @@ libcamera_sources = files([ > >>>> 'v4l2_device.cpp', > >>>> 'v4l2_subdevice.cpp', > >>>> 'v4l2_videodevice.cpp', > >>>> + 'value.cpp', > >>>> 'version.cpp', > >>>> ]) > >>>> > >>>> diff --git a/src/libcamera/value.cpp b/src/libcamera/value.cpp > >>>> new file mode 100644 > >>>> index 000000000000..f713907b38dd > >>>> --- /dev/null > >>>> +++ b/src/libcamera/value.cpp > >>>> @@ -0,0 +1,226 @@ > >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > >>>> +/* > >>>> + * Copyright (C) 2019, Google Inc. > >>>> + * > >>>> + * value.cpp - Abstract value handling > >>>> + */ > >>>> + > >>>> +#include <limits.h> > >>>> +#include <sstream> > >>>> +#include <string> > >>>> + > >>>> +#include <libcamera/value.h> > >>> > >>> Please move this as the first include to ensure that it compiles without > >>> depending on any implicitly included headers. > >> > >> Done. > >> > >>>> + > >>>> +#include "log.h" // For ASSERT() > >>> > >>> You don't need to keep the comment. > >> > >> Removed. > >> > >>>> + > >>>> +/** > >>>> + * \file value.h > >>>> + * \brief Handles variant value types as an abstract object > >>>> + */ > >>>> + > >>>> +namespace libcamera { > >>>> + > >>>> +/** > >>>> + * \enum ValueType > >>>> + * Determines the type of value represented by a \a Value > >>>> + * \var ValueNull > >>>> + * Identifies an unset value > >>>> + * \var ValueBool > >>>> + * Identifies controls storing a boolean value > >>>> + * \var ValueInteger > >>>> + * Identifies controls storing an integer value > >>>> + * \var ValueString > >>>> + * Identifies controls storing a string value > >>> > >>> You're missing the 64-bit integer. > >> > >> Added. > >> > >>>> + */ > >>>> + > >>>> +/** > >>>> + * \class Value > >>>> + * \brief Abstract a value for a common data exchange > >>>> + */ > >>>> + > >>>> +/** > >>>> + * \brief Construct an empty Value. > >>>> + * The Value must be \a set() before being used. > >>>> + */ > >>>> +Value::Value() > >>>> + : type_(ValueNull) > >>>> +{ > >>>> +} > >>>> + > >>>> +/** > >>>> + * \brief Construct a Boolean Value > >>>> + * \param[in] value Boolean value to store > >>>> + */ > >>>> +Value::Value(bool value) > >>>> + : type_(ValueBool), bool_(value) > >>>> +{ > >>>> +} > >>>> + > >>>> +/** > >>>> + * \brief Construct an integer Value > >>>> + * \param[in] value Integer value to store > >>>> + */ > >>>> +Value::Value(int value) > >>>> + : type_(ValueInteger), integer_(value) > >>>> +{ > >>>> +} > >>>> + > >>>> +/** > >>>> + * \brief Construct a string Value > >>>> + * \param[in] value String representation to store > >>>> + */ > >>>> +Value::Value(const char *value) > >>>> + : type_(ValueString), string_(value) > >>>> +{ > >>>> +} > >>>> + > >>>> +/** > >>>> + * \brief Construct a string Value > >>>> + * \param[in] value String representation to store > >>>> + */ > >>>> +Value::Value(const std::string &value) > >>>> + : type_(ValueString), string_(value) > >>>> +{ > >>>> +} > >>>> + > >>>> +/** > >>>> + * \fn Value::type > >>>> + * \brief Return the type of value represented by the object > >>>> + */ > >>>> + > >>>> +/** > >>>> + * \fn Value::isNull > >>>> + * \brief Determines if the Value is initialised > >>> > >>> s/Determines/Determine/ > >>> > >>>> + * \return true if the value type is ValueNull, false otherwise > >>> > >>> s/true/True/ > >>> > >>>> + */ > >>>> + > >>>> +/** > >>>> + * \brief Set the value with a boolean > >>>> + * \param[in] value Boolean value to store > >>>> + */ > >>>> +void Value::set(bool value) > >>>> +{ > >>>> + type_ = ValueBool; > >>>> + bool_ = value; > >>>> +} > >>>> + > >>>> +/** > >>>> + * \brief Set the value with an integer > >>>> + * \param[in] value Integer value to store > >>>> + */ > >>>> +void Value::set(int value) > >>>> +{ > >>>> + type_ = ValueInteger; > >>>> + integer_ = value; > >>>> +} > >>>> + > >>>> +/** > >>>> + * \brief Set the value with a 64 bit integer > >>>> + * \param[in] value Integer value to store > >>>> + */ > >>>> +void Value::set(int64_t value) > >>>> +{ > >>>> + type_ = ValueInteger64; > >>>> + integer64_ = value; > >>>> +} > >>>> + > >>>> +/** > >>>> + * \brief Set the value with a string representation > >>>> + * \param[in] value String value to store > >>>> + */ > >>>> +void Value::set(const char *value) > >>>> +{ > >>>> + type_ = ValueString; > >>>> + string_ = value; > >>>> +} > >>>> + > >>>> +/** > >>>> + * \brief Set the value with a string representation > >>>> + * \param[in] value String value to store > >>>> + */ > >>>> +void Value::set(const std::string &value) > >>>> +{ > >>>> + type_ = ValueString; > >>>> + string_ = value; > >>>> +} > >>>> + > >>>> +/** > >>>> + * \brief Get the boolean value. > >>>> + * > >>>> + * The Value type must be Boolean. > >>>> + */ > >>>> +bool Value::getBool() const > >>>> +{ > >>>> + ASSERT(type_ == ValueBool); > >>> > >>> That's quite dangerous... Isn't there a large risk that the value will > >>> be set to a type and retrieved using a different but compatible type, > >>> and that the issue would only be caught at runtime, possibly in uncommon > >>> cases ? For instance consider the following code > >> > >> That's why I put the assert in - to catch those issues early? > >> > >> Warnings and Error prints will scroll off the logs... > > > > With the condition being unlikely the ASSERT() may only trigger in > > production, which wouldn't be very nice. > > Am I misunderstanding /your/ definition of ASSERT? : > > > * \def ASSERT(condition) > > * \brief Abort program execution if assertion fails > > * > > * If \a condition is false, ASSERT() logs an error message with the Fatal log > > * level and aborts program execution. > > * > > * If the macro NDEBUG is defined before including log.h, ASSERT() generates no > > * code. > > ASSERT should never fire in production, because NDEBUG should be set for > BuildType=Release builds. > > >>> > >>> Value v; > >>> > >>> if (likely(condition)) > >>> value.set(3); > >>> else > >>> value.set(0x100000000ULL); > >>> > >>> ... > >>> > >>> int i = v.getInt(); > >>> > >>> This will cause a crash in the unlikely event that condition is false, > >>> and could get unnoticed through testing. An explicit set API with type > >>> names may help there. Given that the Value class will be used for > >>> controls, I think we'll have to make sure we don't crash, invalid types > >>> will need to be caught and reported. We could also decide than an > >>> invalid type would result in the control taking a default value instead > >>> of crashing (but then this should be logged). > >> > >> I'm not sure taking a default value is appropriate either. I don't think > >> any 'default' would be correct. > >> > >> IMO - the types were important, and the asserts were to ensure strict > >> typing when used. > > > > Could we do it the Qt way, where the get functions return an invalid > > value if the type isn't convertible ? For integers and booleans there's > > no invalid values, so Qt adds an optional bool *ok argument. In practice > > Eurgh ... an 'ok' bool sounds icky... I'll look at the Qt code ... Yes, I don't like it much :-( > > I expect the callers to rarely use it though, so I think we need to pick > > between two error handling behaviours: returning a "default" value (0 > > for integers for instance) or crashing. What's best ? :-) > > Crashing in debug builds only :D and returning an undefined value otherwise. > > (it will be whatever the contents of the union is, with whatever > implicit type conversion occurs, which 'may' be as correct as it can be). Should we return a fixed value instead, to make the behaviour reproducible ? Otherwise this may lead to bugs that are hard to reproduce. > > Now that I wrote this, I realised that ASSERT() will be compiled out in > > release builds. Having it in debug builds is thus likely a good idea, > > but we still need to decide on how to handle release builds. > > Aha, well I didn't need to paste that in above then :) > > Too late ! I'll leave it there :P > > >> If the set() call is changed to setInt() would this make you more > >> comfortable? > > > > A little bit :-) > > Ok - I'll remove the implicit type sets. If we go for returning a fixed default value when the type doesn't match, and if we add conversion between compatible types, then overloading the set() function is fine. > >>>> + > >>>> + return bool_; > >>>> +} > >>>> + > >>>> +/** > >>>> + * \brief Get the integer value. > >>>> + * > >>>> + * The Value type must be Integer. > >>>> + */ > >>>> +int Value::getInt() const > >>>> +{ > >>>> + ASSERT(type_ == ValueInteger); > >>>> + > >>>> + return integer_; > >>>> +} > >>>> + > >>>> +/** > >>>> + * \brief Get the 64bit integer value. > >>>> + * > >>>> + * The Value type must be Integer64. > >>>> + */ > >>>> +int Value::getInt64() const > >>>> +{ > >>>> + ASSERT(type_ == ValueInteger64); > >>>> + > >>>> + return integer64_; > >>>> +} > >>>> + > >>>> +/** > >>>> + * \brief Get the string value. > >>>> + * > >>>> + * The Value type must be String. > >>>> + */ > >>>> +std::string Value::getString() const > >>>> +{ > >>>> + ASSERT(type_ == ValueString); > >>>> + > >>>> + return string_; > >>>> +} > >>>> + > >>>> +/** > >>>> + * \brief Prepare a string representation of the Value > >>>> + */ > >>>> +std::string Value::toString() const > >>>> +{ > >>>> + switch (type_) { > >>>> + case ValueNull: > >>>> + return "<NULL>"; > >>>> + case ValueBool: > >>>> + return bool_ ? "True" : "False"; > >>>> + case ValueInteger: > >>>> + return std::to_string(integer_); > >>>> + case ValueInteger64: > >>>> + return std::to_string(integer64_); > >>>> + case ValueString: > >>>> + return string_; > >>>> + } > >>>> + > >>>> + /* Unreachable */ > >>>> + return "<ValueType Error>"; > >>> > >>> Do you need this ? Does the compiler complain when the switch-case > >>> handles all the cases ? > >> > >> Yup. > >> > >> ../src/libcamera/value.cpp: In member function ‘std::__cxx11::string > >> libcamera::Value::toString() const’: > >> ../src/libcamera/value.cpp:212:1: error: control reaches end of non-void > >> function [-Werror=return-type] > >> } > >> ^ > >> cc1plus: all warnings being treated as errors > >> > >> I can move it to a 'default:' statement which then removes the warning - > >> but should still be redundant/ unreachable. > > > > A default statement sounds nice, but I think that an error after the > > switch() is better as in that case the compiler should warn you if you > > add a new enum entry without adding a corresponding case. > > I agree, I'd rather the compiler tell us if we have a missing enum type. > > >>>> +} > >>>> + > >>>> +/** > >>>> + * \brief Provide a string stream representation of the Value \a value to > >>>> + * the \a stream. > >>> > >>> It's not a string stream, it's an output stream (and you should include > >>> ostream instead of sstream). > >>> > >>>> + */ > >>>> +std::ostream &operator<<(std::ostream &stream, const Value &value) > >>>> +{ > >>>> + return stream << value.toString(); > >>>> +} > >>>> + > >>>> +} /* namespace libcamera */
diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build index 201832105457..eb2211ae1fc3 100644 --- a/include/libcamera/meson.build +++ b/include/libcamera/meson.build @@ -12,6 +12,7 @@ libcamera_api = files([ 'signal.h', 'stream.h', 'timer.h', + 'value.h', 'version.h', ]) diff --git a/include/libcamera/value.h b/include/libcamera/value.h new file mode 100644 index 000000000000..00c5d53d5cc0 --- /dev/null +++ b/include/libcamera/value.h @@ -0,0 +1,63 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * value.h - Abstract value handling + */ + +#include <map> + +#ifndef __LIBCAMERA_VALUE_H__ +#define __LIBCAMERA_VALUE_H__ + +namespace libcamera { + +enum ValueType : uint8_t { + ValueNull, + ValueBool, + ValueInteger, + ValueInteger64, + ValueString, +}; + +class Value +{ +public: + Value(); + Value(bool value); + Value(int value); + Value(const char *value); + Value(const std::string &value); + + ValueType type() const { return type_; }; + bool isNull() const { return type_ == ValueNull; }; + + void set(bool value); + void set(int value); + void set(int64_t value); + void set(const char *value); + void set(const std::string &value); + + bool getBool() const; + int getInt() const; + int getInt64() const; + std::string getString() const; + + std::string toString() const; + +private: + ValueType type_; + + union { + bool bool_; + int integer_; + int64_t integer64_; + }; + std::string string_; +}; + +std::ostream &operator<<(std::ostream &stream, const Value &value); + +} /* namespace libcamera */ + +#endif /* __LIBCAMERA_VALUE_H__ */ diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build index 68c7ce14b5b4..8e68373118df 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -27,6 +27,7 @@ libcamera_sources = files([ 'v4l2_device.cpp', 'v4l2_subdevice.cpp', 'v4l2_videodevice.cpp', + 'value.cpp', 'version.cpp', ]) diff --git a/src/libcamera/value.cpp b/src/libcamera/value.cpp new file mode 100644 index 000000000000..f713907b38dd --- /dev/null +++ b/src/libcamera/value.cpp @@ -0,0 +1,226 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * value.cpp - Abstract value handling + */ + +#include <limits.h> +#include <sstream> +#include <string> + +#include <libcamera/value.h> + +#include "log.h" // For ASSERT() + +/** + * \file value.h + * \brief Handles variant value types as an abstract object + */ + +namespace libcamera { + +/** + * \enum ValueType + * Determines the type of value represented by a \a Value + * \var ValueNull + * Identifies an unset value + * \var ValueBool + * Identifies controls storing a boolean value + * \var ValueInteger + * Identifies controls storing an integer value + * \var ValueString + * Identifies controls storing a string value + */ + +/** + * \class Value + * \brief Abstract a value for a common data exchange + */ + +/** + * \brief Construct an empty Value. + * The Value must be \a set() before being used. + */ +Value::Value() + : type_(ValueNull) +{ +} + +/** + * \brief Construct a Boolean Value + * \param[in] value Boolean value to store + */ +Value::Value(bool value) + : type_(ValueBool), bool_(value) +{ +} + +/** + * \brief Construct an integer Value + * \param[in] value Integer value to store + */ +Value::Value(int value) + : type_(ValueInteger), integer_(value) +{ +} + +/** + * \brief Construct a string Value + * \param[in] value String representation to store + */ +Value::Value(const char *value) + : type_(ValueString), string_(value) +{ +} + +/** + * \brief Construct a string Value + * \param[in] value String representation to store + */ +Value::Value(const std::string &value) + : type_(ValueString), string_(value) +{ +} + +/** + * \fn Value::type + * \brief Return the type of value represented by the object + */ + +/** + * \fn Value::isNull + * \brief Determines if the Value is initialised + * \return true if the value type is ValueNull, false otherwise + */ + +/** + * \brief Set the value with a boolean + * \param[in] value Boolean value to store + */ +void Value::set(bool value) +{ + type_ = ValueBool; + bool_ = value; +} + +/** + * \brief Set the value with an integer + * \param[in] value Integer value to store + */ +void Value::set(int value) +{ + type_ = ValueInteger; + integer_ = value; +} + +/** + * \brief Set the value with a 64 bit integer + * \param[in] value Integer value to store + */ +void Value::set(int64_t value) +{ + type_ = ValueInteger64; + integer64_ = value; +} + +/** + * \brief Set the value with a string representation + * \param[in] value String value to store + */ +void Value::set(const char *value) +{ + type_ = ValueString; + string_ = value; +} + +/** + * \brief Set the value with a string representation + * \param[in] value String value to store + */ +void Value::set(const std::string &value) +{ + type_ = ValueString; + string_ = value; +} + +/** + * \brief Get the boolean value. + * + * The Value type must be Boolean. + */ +bool Value::getBool() const +{ + ASSERT(type_ == ValueBool); + + return bool_; +} + +/** + * \brief Get the integer value. + * + * The Value type must be Integer. + */ +int Value::getInt() const +{ + ASSERT(type_ == ValueInteger); + + return integer_; +} + +/** + * \brief Get the 64bit integer value. + * + * The Value type must be Integer64. + */ +int Value::getInt64() const +{ + ASSERT(type_ == ValueInteger64); + + return integer64_; +} + +/** + * \brief Get the string value. + * + * The Value type must be String. + */ +std::string Value::getString() const +{ + ASSERT(type_ == ValueString); + + return string_; +} + +/** + * \brief Prepare a string representation of the Value + */ +std::string Value::toString() const +{ + switch (type_) { + case ValueNull: + return "<NULL>"; + case ValueBool: + return bool_ ? "True" : "False"; + case ValueInteger: + return std::to_string(integer_); + case ValueInteger64: + return std::to_string(integer64_); + case ValueString: + return string_; + } + + /* Unreachable */ + return "<ValueType Error>"; +} + +/** + * \brief Provide a string stream representation of the Value \a value to + * the \a stream. + */ +std::ostream &operator<<(std::ostream &stream, const Value &value) +{ + return stream << value.toString(); +} + +} /* namespace libcamera */
To facilitate passing typed data generically, implement a class which stores the type and value as efficiently as possible. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- include/libcamera/meson.build | 1 + include/libcamera/value.h | 63 ++++++++++ src/libcamera/meson.build | 1 + src/libcamera/value.cpp | 226 ++++++++++++++++++++++++++++++++++ 4 files changed, 291 insertions(+) create mode 100644 include/libcamera/value.h create mode 100644 src/libcamera/value.cpp