[{"id":1971,"web_url":"https://patchwork.libcamera.org/comment/1971/","msgid":"<20190622223943.GJ8156@pendragon.ideasonboard.com>","date":"2019-06-22T22:39:43","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/9] libcamera: Value: Provide\n\tabstract value class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Fri, Jun 21, 2019 at 05:13:53PM +0100, Kieran Bingham wrote:\n> To facilitate passing typed data generically, implement a class which stores\n> the type and value as efficiently as possible.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/meson.build | 1 +\n> include/libcamera/value.h | 63 ++++++++++\n> src/libcamera/meson.build | 1 +\n> src/libcamera/value.cpp | 226 ++++++++++++++++++++++++++++++++++\n> 4 files changed, 291 insertions(+)\n> create mode 100644 include/libcamera/value.h\n> create mode 100644 src/libcamera/value.cpp\n> \n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 201832105457..eb2211ae1fc3 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -12,6 +12,7 @@ libcamera_api = files([\n> 'signal.h',\n> 'stream.h',\n> 'timer.h',\n> + 'value.h',\n> 'version.h',\n> ])\n> \n> diff --git a/include/libcamera/value.h b/include/libcamera/value.h\n> new file mode 100644\n> index 000000000000..00c5d53d5cc0\n> --- /dev/null\n> +++ b/include/libcamera/value.h\n> @@ -0,0 +1,63 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * value.h - Abstract value handling\n> + */\n> +\n> +#include \n> +\n> +#ifndef __LIBCAMERA_VALUE_H__\n> +#define __LIBCAMERA_VALUE_H__\n> +\n> +namespace libcamera {\n> +\n> +enum ValueType : uint8_t {\n\nDo we need an explicit uint8_t here ? It won't save memory as the next\nmember of the Value class will be aligned to a 64-bits boundary.\n\n> +\tValueNull,\n\nLet's call it ValueUnset or ValueNone. Null has a well established\nmeaning.\n\n> +\tValueBool,\n> +\tValueInteger,\n> +\tValueInteger64,\n> +\tValueString,\n> +};\n> +\n> +class Value\n\nI think I would call this ControlValue for now, and then expand it to a\nmore generic solution if needed, instead of trying to be too generic to\nstart with.\n\n> +{\n> +public:\n> +\tValue();\n> +\tValue(bool value);\n> +\tValue(int value);\n> +\tValue(const char *value);\n> +\tValue(const std::string &value);\n> +\n> +\tValueType type() const { return type_; };\n> +\tbool isNull() const { return type_ == ValueNull; };\n\nisUnset() ?\n\n> +\n> +\tvoid set(bool value);\n> +\tvoid set(int value);\n> +\tvoid set(int64_t value);\n\nThis is asking for trouble, you will often end up with the wrong integer\ntype without even noticing, unless you can remember and apply flawlessly\nthe C++ integer types implicit conversions rules.\n\n> +\tvoid set(const char *value);\n> +\tvoid set(const std::string &value);\n> +\n> +\tbool getBool() const;\n> +\tint getInt() const;\n> +\tint getInt64() const;\n\nThis should return a 64-bit integer.\n\n> +\tstd::string getString() const;\n> +\n> +\tstd::string toString() const;\n> +\n> +private:\n> +\tValueType type_;\n> +\n> +\tunion {\n> +\t\tbool bool_;\n> +\t\tint integer_;\n> +\t\tint64_t integer64_;\n> +\t};\n> +\tstd::string string_;\n\nAs you've noticed, usage of an std::string makes it impossible to make\nthe string_ member part of the value union, which reduces memory and CPU\nefficiency. Every integer value will need to construct an std::string.\nFor this reason I would store the data for complex types as a pointer\ninstead, and allocate it dynamically. It could be a pointer to a\nstd::string, or just a const char *. The latter would likely be more\nefficient.\n\n> +};\n> +\n> +std::ostream &operator<<(std::ostream &stream, const Value &value);\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_VALUE_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 68c7ce14b5b4..8e68373118df 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -27,6 +27,7 @@ libcamera_sources = files([\n> 'v4l2_device.cpp',\n> 'v4l2_subdevice.cpp',\n> 'v4l2_videodevice.cpp',\n> + 'value.cpp',\n> 'version.cpp',\n> ])\n> \n> diff --git a/src/libcamera/value.cpp b/src/libcamera/value.cpp\n> new file mode 100644\n> index 000000000000..f713907b38dd\n> --- /dev/null\n> +++ b/src/libcamera/value.cpp\n> @@ -0,0 +1,226 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * value.cpp - Abstract value handling\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n\nPlease move this as the first include to ensure that it compiles without\ndepending on any implicitly included headers.\n\n> +\n> +#include \"log.h\" // For ASSERT()\n\nYou don't need to keep the comment.\n\n> +\n> +/**\n> + * \\file value.h\n> + * \\brief Handles variant value types as an abstract object\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\enum ValueType\n> + * Determines the type of value represented by a \\a Value\n> + * \\var ValueNull\n> + * Identifies an unset value\n> + * \\var ValueBool\n> + * Identifies controls storing a boolean value\n> + * \\var ValueInteger\n> + * Identifies controls storing an integer value\n> + * \\var ValueString\n> + * Identifies controls storing a string value\n\nYou're missing the 64-bit integer.\n\n> + */\n> +\n> +/**\n> + * \\class Value\n> + * \\brief Abstract a value for a common data exchange\n> + */\n> +\n> +/**\n> + * \\brief Construct an empty Value.\n> + * The Value must be \\a set() before being used.\n> + */\n> +Value::Value()\n> +\t: type_(ValueNull)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Construct a Boolean Value\n> + * \\param[in] value Boolean value to store\n> + */\n> +Value::Value(bool value)\n> +\t: type_(ValueBool), bool_(value)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Construct an integer Value\n> + * \\param[in] value Integer value to store\n> + */\n> +Value::Value(int value)\n> +\t: type_(ValueInteger), integer_(value)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Construct a string Value\n> + * \\param[in] value String representation to store\n> + */\n> +Value::Value(const char *value)\n> +\t: type_(ValueString), string_(value)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Construct a string Value\n> + * \\param[in] value String representation to store\n> + */\n> +Value::Value(const std::string &value)\n> +\t: type_(ValueString), string_(value)\n> +{\n> +}\n> +\n> +/**\n> + * \\fn Value::type\n> + * \\brief Return the type of value represented by the object\n> + */\n> +\n> +/**\n> + * \\fn Value::isNull\n> + * \\brief Determines if the Value is initialised\n\ns/Determines/Determine/\n\n> + * \\return true if the value type is ValueNull, false otherwise\n\ns/true/True/\n\n> + */\n> +\n> +/**\n> + * \\brief Set the value with a boolean\n> + * \\param[in] value Boolean value to store\n> + */\n> +void Value::set(bool value)\n> +{\n> +\ttype_ = ValueBool;\n> +\tbool_ = value;\n> +}\n> +\n> +/**\n> + * \\brief Set the value with an integer\n> + * \\param[in] value Integer value to store\n> + */\n> +void Value::set(int value)\n> +{\n> +\ttype_ = ValueInteger;\n> +\tinteger_ = value;\n> +}\n> +\n> +/**\n> + * \\brief Set the value with a 64 bit integer\n> + * \\param[in] value Integer value to store\n> + */\n> +void Value::set(int64_t value)\n> +{\n> +\ttype_ = ValueInteger64;\n> +\tinteger64_ = value;\n> +}\n> +\n> +/**\n> + * \\brief Set the value with a string representation\n> + * \\param[in] value String value to store\n> + */\n> +void Value::set(const char *value)\n> +{\n> +\ttype_ = ValueString;\n> +\tstring_ = value;\n> +}\n> +\n> +/**\n> + * \\brief Set the value with a string representation\n> + * \\param[in] value String value to store\n> + */\n> +void Value::set(const std::string &value)\n> +{\n> +\ttype_ = ValueString;\n> +\tstring_ = value;\n> +}\n> +\n> +/**\n> + * \\brief Get the boolean value.\n> + *\n> + * The Value type must be Boolean.\n> + */\n> +bool Value::getBool() const\n> +{\n> +\tASSERT(type_ == ValueBool);\n\nThat's quite dangerous... Isn't there a large risk that the value will\nbe set to a type and retrieved using a different but compatible type,\nand that the issue would only be caught at runtime, possibly in uncommon\ncases ? For instance consider the following code\n\n\n\tValue v;\n\n\tif (likely(condition))\n\t\tvalue.set(3);\n\telse\n\t\tvalue.set(0x100000000ULL);\n\n\t...\n\n\tint i = v.getInt();\n\nThis will cause a crash in the unlikely event that condition is false,\nand could get unnoticed through testing. An explicit set API with type\nnames may help there. Given that the Value class will be used for\ncontrols, I think we'll have to make sure we don't crash, invalid types\nwill need to be caught and reported. We could also decide than an\ninvalid type would result in the control taking a default value instead\nof crashing (but then this should be logged).\n\n> +\n> +\treturn bool_;\n> +}\n> +\n> +/**\n> + * \\brief Get the integer value.\n> + *\n> + * The Value type must be Integer.\n> + */\n> +int Value::getInt() const\n> +{\n> +\tASSERT(type_ == ValueInteger);\n> +\n> +\treturn integer_;\n> +}\n> +\n> +/**\n> + * \\brief Get the 64bit integer value.\n> + *\n> + * The Value type must be Integer64.\n> + */\n> +int Value::getInt64() const\n> +{\n> +\tASSERT(type_ == ValueInteger64);\n> +\n> +\treturn integer64_;\n> +}\n> +\n> +/**\n> + * \\brief Get the string value.\n> + *\n> + * The Value type must be String.\n> + */\n> +std::string Value::getString() const\n> +{\n> +\tASSERT(type_ == ValueString);\n> +\n> +\treturn string_;\n> +}\n> +\n> +/**\n> + * \\brief Prepare a string representation of the Value\n> + */\n> +std::string Value::toString() const\n> +{\n> +\tswitch (type_) {\n> +\tcase ValueNull:\n> +\t\treturn \"\";\n> +\tcase ValueBool:\n> +\t\treturn bool_ ? \"True\" : \"False\";\n> +\tcase ValueInteger:\n> +\t\treturn std::to_string(integer_);\n> +\tcase ValueInteger64:\n> +\t\treturn std::to_string(integer64_);\n> +\tcase ValueString:\n> +\t\treturn string_;\n> +\t}\n> +\n> +\t/* Unreachable */\n> +\treturn \"\";\n\nDo you need this ? Does the compiler complain when the switch-case\nhandles all the cases ?\n\n> +}\n> +\n> +/**\n> + * \\brief Provide a string stream representation of the Value \\a value to\n> + * the \\a stream.\n\nIt's not a string stream, it's an output stream (and you should include\nostream instead of sstream).\n\n> + */\n> +std::ostream &operator<<(std::ostream &stream, const Value &value)\n> +{\n> +\treturn stream << value.toString();\n> +}\n> +\n> +} /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 737386002B\n\tfor ;\n\tSun, 23 Jun 2019 00:42:15 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E518467;\n\tSun, 23 Jun 2019 00:42:14 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561243335;\n\tbh=CnmmFVnRZ2LA1suaRzeJamX1xFojRVhVJi9XZAAOHVA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fLhw80HqjvMg2x0BcrOFV0xblU2jpdBx2BnE7KcN4o5JBG5V+lhAV/2k51bKs6p9F\n\tfR7izC1lCUtw1iw9O5fhA+Mepw2TBZyCnZhssEuBgiyMt8PaiJjqZ5+7xORNlrKMd1\n\tjqd+SYHa1Q3gtDpw+nOQkhG6N2j6qlM/S1r8Dh7E=","Date":"Sun, 23 Jun 2019 01:39:43 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190622223943.GJ8156@pendragon.ideasonboard.com>","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190621161401.28337-2-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/9] libcamera: Value: Provide\n\tabstract value class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 22 Jun 2019 22:42:15 -0000"}},{"id":1994,"web_url":"https://patchwork.libcamera.org/comment/1994/","msgid":"","date":"2019-06-24T09:09:06","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/9] libcamera: Value: Provide\n\tabstract value class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 22/06/2019 23:39, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Fri, Jun 21, 2019 at 05:13:53PM +0100, Kieran Bingham wrote:\n>> To facilitate passing typed data generically, implement a class which stores\n>> the type and value as efficiently as possible.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> include/libcamera/meson.build | 1 +\n>> include/libcamera/value.h | 63 ++++++++++\n>> src/libcamera/meson.build | 1 +\n>> src/libcamera/value.cpp | 226 ++++++++++++++++++++++++++++++++++\n>> 4 files changed, 291 insertions(+)\n>> create mode 100644 include/libcamera/value.h\n>> create mode 100644 src/libcamera/value.cpp\n>>\n>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n>> index 201832105457..eb2211ae1fc3 100644\n>> --- a/include/libcamera/meson.build\n>> +++ b/include/libcamera/meson.build\n>> @@ -12,6 +12,7 @@ libcamera_api = files([\n>> 'signal.h',\n>> 'stream.h',\n>> 'timer.h',\n>> + 'value.h',\n>> 'version.h',\n>> ])\n>> \n>> diff --git a/include/libcamera/value.h b/include/libcamera/value.h\n>> new file mode 100644\n>> index 000000000000..00c5d53d5cc0\n>> --- /dev/null\n>> +++ b/include/libcamera/value.h\n>> @@ -0,0 +1,63 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2019, Google Inc.\n>> + *\n>> + * value.h - Abstract value handling\n>> + */\n>> +\n>> +#include \n>> +\n>> +#ifndef __LIBCAMERA_VALUE_H__\n>> +#define __LIBCAMERA_VALUE_H__\n>> +\n>> +namespace libcamera {\n>> +\n>> +enum ValueType : uint8_t {\n> \n> Do we need an explicit uint8_t here ? It won't save memory as the next\n> member of the Value class will be aligned to a 64-bits boundary.\n\nTrue, but ValueType might be used in other locations too? Although then\nthey'd only be used for checking the type - so storage isn't really an\nissue there either.\n\nMy actual reason for playing around with specifying the types on enums\nwas to explore the effects on the ABI.\n\nThe ValueType enum should certainly never exceed 255 entries, which is\none benefit of being explicitly smaller - but I do think on this\noccasion it is completely redundant so I'll just remove it now.\n\n\n> \n>> +\tValueNull,\n> \n> Let's call it ValueUnset or ValueNone. Null has a well established\n> meaning.\n\nI started with ValueNone ... but moved to ValueNull ... but I'm happy to\ngo back again :-)\n\n\n> \n>> +\tValueBool,\n>> +\tValueInteger,\n>> +\tValueInteger64,\n>> +\tValueString,\n>> +};\n>> +\n>> +class Value\n> \n> I think I would call this ControlValue for now, and then expand it to a\n> more generic solution if needed, instead of trying to be too generic to\n> start with.\n\nAnd this whole class came from ControlValue\n\n\n> \n>> +{\n>> +public:\n>> +\tValue();\n>> +\tValue(bool value);\n>> +\tValue(int value);\n>> +\tValue(const char *value);\n>> +\tValue(const std::string &value);\n>> +\n>> +\tValueType type() const { return type_; };\n>> +\tbool isNull() const { return type_ == ValueNull; };\n> \n> isUnset() ?\n\nWell if it's not Null anymore, shouldn't it be isNone?\n\n\nOr do you prefer (Un)'set' to mirror the set() call.\n\n\n>> +\n>> +\tvoid set(bool value);\n>> +\tvoid set(int value);\n>> +\tvoid set(int64_t value);\n> \n> This is asking for trouble, you will often end up with the wrong integer\n> type without even noticing, unless you can remember and apply flawlessly\n> the C++ integer types implicit conversions rules.\n\nHrm... So types in the Function name?\n\n\n>> +\tvoid set(const char *value);\n>> +\tvoid set(const std::string &value);\n>> +\n>> +\tbool getBool() const;\n>> +\tint getInt() const;\n>> +\tint getInt64() const;\n> \n> This should return a 64-bit integer.\n\nIndeed.\n\n> \n>> +\tstd::string getString() const;\n>> +\n>> +\tstd::string toString() const;\n>> +\n>> +private:\n>> +\tValueType type_;\n>> +\n>> +\tunion {\n>> +\t\tbool bool_;\n>> +\t\tint integer_;\n>> +\t\tint64_t integer64_;\n>> +\t};\n>> +\tstd::string string_;\n> \n> As you've noticed, usage of an std::string makes it impossible to make\n> the string_ member part of the value union, which reduces memory and CPU\n> efficiency. Every integer value will need to construct an std::string.\n> For this reason I would store the data for complex types as a pointer\n> instead, and allocate it dynamically. It could be a pointer to a\n> std::string, or just a const char *. The latter would likely be more\n> efficient.\n\n\nIndeed. I think this is looking to get dropped rather than converted at\nthe moment.\n\nI wanted to make the Value class a generic re-usable class that could be\nre-used in the existing Option parsers, and V4L2Controls, because we are\nalready repeating the same patterns across the code base.\n\nBut Jacopo doesn't want to make use of it there either ...\n\n\n\n>> +};\n>> +\n>> +std::ostream &operator<<(std::ostream &stream, const Value &value);\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_VALUE_H__ */\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index 68c7ce14b5b4..8e68373118df 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -27,6 +27,7 @@ libcamera_sources = files([\n>> 'v4l2_device.cpp',\n>> 'v4l2_subdevice.cpp',\n>> 'v4l2_videodevice.cpp',\n>> + 'value.cpp',\n>> 'version.cpp',\n>> ])\n>> \n>> diff --git a/src/libcamera/value.cpp b/src/libcamera/value.cpp\n>> new file mode 100644\n>> index 000000000000..f713907b38dd\n>> --- /dev/null\n>> +++ b/src/libcamera/value.cpp\n>> @@ -0,0 +1,226 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2019, Google Inc.\n>> + *\n>> + * value.cpp - Abstract value handling\n>> + */\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n> \n> Please move this as the first include to ensure that it compiles without\n> depending on any implicitly included headers.\n\nDone.\n\n> \n>> +\n>> +#include \"log.h\" // For ASSERT()\n> \n> You don't need to keep the comment.\n\nRemoved.\n\n\n> \n>> +\n>> +/**\n>> + * \\file value.h\n>> + * \\brief Handles variant value types as an abstract object\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +/**\n>> + * \\enum ValueType\n>> + * Determines the type of value represented by a \\a Value\n>> + * \\var ValueNull\n>> + * Identifies an unset value\n>> + * \\var ValueBool\n>> + * Identifies controls storing a boolean value\n>> + * \\var ValueInteger\n>> + * Identifies controls storing an integer value\n>> + * \\var ValueString\n>> + * Identifies controls storing a string value\n> \n> You're missing the 64-bit integer.\n> \n\nAdded.\n\n>> + */\n>> +\n>> +/**\n>> + * \\class Value\n>> + * \\brief Abstract a value for a common data exchange\n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct an empty Value.\n>> + * The Value must be \\a set() before being used.\n>> + */\n>> +Value::Value()\n>> +\t: type_(ValueNull)\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\brief Construct a Boolean Value\n>> + * \\param[in] value Boolean value to store\n>> + */\n>> +Value::Value(bool value)\n>> +\t: type_(ValueBool), bool_(value)\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\brief Construct an integer Value\n>> + * \\param[in] value Integer value to store\n>> + */\n>> +Value::Value(int value)\n>> +\t: type_(ValueInteger), integer_(value)\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\brief Construct a string Value\n>> + * \\param[in] value String representation to store\n>> + */\n>> +Value::Value(const char *value)\n>> +\t: type_(ValueString), string_(value)\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\brief Construct a string Value\n>> + * \\param[in] value String representation to store\n>> + */\n>> +Value::Value(const std::string &value)\n>> +\t: type_(ValueString), string_(value)\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\fn Value::type\n>> + * \\brief Return the type of value represented by the object\n>> + */\n>> +\n>> +/**\n>> + * \\fn Value::isNull\n>> + * \\brief Determines if the Value is initialised\n> \n> s/Determines/Determine/\n> \n>> + * \\return true if the value type is ValueNull, false otherwise\n> \n> s/true/True/\n> \n>> + */\n>> +\n>> +/**\n>> + * \\brief Set the value with a boolean\n>> + * \\param[in] value Boolean value to store\n>> + */\n>> +void Value::set(bool value)\n>> +{\n>> +\ttype_ = ValueBool;\n>> +\tbool_ = value;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Set the value with an integer\n>> + * \\param[in] value Integer value to store\n>> + */\n>> +void Value::set(int value)\n>> +{\n>> +\ttype_ = ValueInteger;\n>> +\tinteger_ = value;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Set the value with a 64 bit integer\n>> + * \\param[in] value Integer value to store\n>> + */\n>> +void Value::set(int64_t value)\n>> +{\n>> +\ttype_ = ValueInteger64;\n>> +\tinteger64_ = value;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Set the value with a string representation\n>> + * \\param[in] value String value to store\n>> + */\n>> +void Value::set(const char *value)\n>> +{\n>> +\ttype_ = ValueString;\n>> +\tstring_ = value;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Set the value with a string representation\n>> + * \\param[in] value String value to store\n>> + */\n>> +void Value::set(const std::string &value)\n>> +{\n>> +\ttype_ = ValueString;\n>> +\tstring_ = value;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Get the boolean value.\n>> + *\n>> + * The Value type must be Boolean.\n>> + */\n>> +bool Value::getBool() const\n>> +{\n>> +\tASSERT(type_ == ValueBool);\n> \n> That's quite dangerous... Isn't there a large risk that the value will\n> be set to a type and retrieved using a different but compatible type,\n> and that the issue would only be caught at runtime, possibly in uncommon\n> cases ? For instance consider the following code\n\n\nThat's why I put the assert in - to catch those issues early?\n\nWarnings and Error prints will scroll off the logs...\n\n\n> \n> \tValue v;\n> \n> \tif (likely(condition))\n> \t\tvalue.set(3);\n> \telse\n> \t\tvalue.set(0x100000000ULL);\n> \n> \t...\n> \n> \tint i = v.getInt();\n> \n> This will cause a crash in the unlikely event that condition is false,\n> and could get unnoticed through testing. An explicit set API with type\n> names may help there. Given that the Value class will be used for\n> controls, I think we'll have to make sure we don't crash, invalid types\n> will need to be caught and reported. We could also decide than an\n> invalid type would result in the control taking a default value instead\n> of crashing (but then this should be logged).\n\nI'm not sure taking a default value is appropriate either. I don't think\nany 'default' would be correct.\n\nIMO - the types were important, and the asserts were to ensure strict\ntyping when used.\n\nIf the set() call is changed to setInt() would this make you more\ncomfortable?\n\n> \n>> +\n>> +\treturn bool_;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Get the integer value.\n>> + *\n>> + * The Value type must be Integer.\n>> + */\n>> +int Value::getInt() const\n>> +{\n>> +\tASSERT(type_ == ValueInteger);\n>> +\n>> +\treturn integer_;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Get the 64bit integer value.\n>> + *\n>> + * The Value type must be Integer64.\n>> + */\n>> +int Value::getInt64() const\n>> +{\n>> +\tASSERT(type_ == ValueInteger64);\n>> +\n>> +\treturn integer64_;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Get the string value.\n>> + *\n>> + * The Value type must be String.\n>> + */\n>> +std::string Value::getString() const\n>> +{\n>> +\tASSERT(type_ == ValueString);\n>> +\n>> +\treturn string_;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Prepare a string representation of the Value\n>> + */\n>> +std::string Value::toString() const\n>> +{\n>> +\tswitch (type_) {\n>> +\tcase ValueNull:\n>> +\t\treturn \"\";\n>> +\tcase ValueBool:\n>> +\t\treturn bool_ ? \"True\" : \"False\";\n>> +\tcase ValueInteger:\n>> +\t\treturn std::to_string(integer_);\n>> +\tcase ValueInteger64:\n>> +\t\treturn std::to_string(integer64_);\n>> +\tcase ValueString:\n>> +\t\treturn string_;\n>> +\t}\n>> +\n>> +\t/* Unreachable */\n>> +\treturn \"\";\n> \n> Do you need this ? Does the compiler complain when the switch-case\n> handles all the cases ?\n> \n\nYup.\n\n../src/libcamera/value.cpp: In member function ‘std::__cxx11::string\nlibcamera::Value::toString() const’:\n../src/libcamera/value.cpp:212:1: error: control reaches end of non-void\nfunction [-Werror=return-type]\n }\n ^\ncc1plus: all warnings being treated as errors\n\nI can move it to a 'default:' statement which then removes the warning -\nbut should still be redundant/ unreachable.\n\n\n\n>> +}\n>> +\n>> +/**\n>> + * \\brief Provide a string stream representation of the Value \\a value to\n>> + * the \\a stream.\n> \n> It's not a string stream, it's an output stream (and you should include\n> ostream instead of sstream).\n> \n>> + */\n>> +std::ostream &operator<<(std::ostream &stream, const Value &value)\n>> +{\n>> +\treturn stream << value.toString();\n>> +}\n>> +\n>> +} /* namespace libcamera */\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A27E60C29\n\tfor ;\n\tMon, 24 Jun 2019 11:09:09 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EBD3E323;\n\tMon, 24 Jun 2019 11:09:08 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561367349;\n\tbh=W2HE2zXr1ZQ0D5emCEfZMh/V9b09yOOeOgmM22Qn9xU=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=XHBPjktei16a619EaBn5qc2OWkT4mfOj0Ymx/t5MQziSfcLbZ40wAJPMoZH0conmZ\n\trbFo7pah963e2+Iu9XlrfR72jkYsDOF3WCR35wb+XguZks2ImqTmLMOyXLqg9phlQb\n\tEdTEuda0mu325tk1l8mnEMmhg8ivIneVK5oeH3VM=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-2-kieran.bingham@ideasonboard.com>\n\t<20190622223943.GJ8156@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"","Date":"Mon, 24 Jun 2019 10:09:06 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190622223943.GJ8156@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/9] libcamera: Value: Provide\n\tabstract value class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 09:09:09 -0000"}},{"id":2000,"web_url":"https://patchwork.libcamera.org/comment/2000/","msgid":"<20190624120908.GH5737@pendragon.ideasonboard.com>","date":"2019-06-24T12:09:08","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/9] libcamera: Value: Provide\n\tabstract value class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Jun 24, 2019 at 10:09:06AM +0100, Kieran Bingham wrote:\n> On 22/06/2019 23:39, Laurent Pinchart wrote:\n> > On Fri, Jun 21, 2019 at 05:13:53PM +0100, Kieran Bingham wrote:\n> >> To facilitate passing typed data generically, implement a class which stores\n> >> the type and value as efficiently as possible.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> include/libcamera/meson.build | 1 +\n> >> include/libcamera/value.h | 63 ++++++++++\n> >> src/libcamera/meson.build | 1 +\n> >> src/libcamera/value.cpp | 226 ++++++++++++++++++++++++++++++++++\n> >> 4 files changed, 291 insertions(+)\n> >> create mode 100644 include/libcamera/value.h\n> >> create mode 100644 src/libcamera/value.cpp\n> >>\n> >> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> >> index 201832105457..eb2211ae1fc3 100644\n> >> --- a/include/libcamera/meson.build\n> >> +++ b/include/libcamera/meson.build\n> >> @@ -12,6 +12,7 @@ libcamera_api = files([\n> >> 'signal.h',\n> >> 'stream.h',\n> >> 'timer.h',\n> >> + 'value.h',\n> >> 'version.h',\n> >> ])\n> >> \n> >> diff --git a/include/libcamera/value.h b/include/libcamera/value.h\n> >> new file mode 100644\n> >> index 000000000000..00c5d53d5cc0\n> >> --- /dev/null\n> >> +++ b/include/libcamera/value.h\n> >> @@ -0,0 +1,63 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2019, Google Inc.\n> >> + *\n> >> + * value.h - Abstract value handling\n> >> + */\n> >> +\n> >> +#include \n> >> +\n> >> +#ifndef __LIBCAMERA_VALUE_H__\n> >> +#define __LIBCAMERA_VALUE_H__\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +enum ValueType : uint8_t {\n> > \n> > Do we need an explicit uint8_t here ? It won't save memory as the next\n> > member of the Value class will be aligned to a 64-bits boundary.\n> \n> True, but ValueType might be used in other locations too? Although then\n> they'd only be used for checking the type - so storage isn't really an\n> issue there either.\n> \n> My actual reason for playing around with specifying the types on enums\n> was to explore the effects on the ABI.\n> \n> The ValueType enum should certainly never exceed 255 entries, which is\n> one benefit of being explicitly smaller - but I do think on this\n> occasion it is completely redundant so I'll just remove it now.\n> \n> >> +\tValueNull,\n> > \n> > Let's call it ValueUnset or ValueNone. Null has a well established\n> > meaning.\n> \n> I started with ValueNone ... but moved to ValueNull ... but I'm happy to\n> go back again :-)\n> \n> >> +\tValueBool,\n> >> +\tValueInteger,\n> >> +\tValueInteger64,\n> >> +\tValueString,\n> >> +};\n> >> +\n> >> +class Value\n> > \n> > I think I would call this ControlValue for now, and then expand it to a\n> > more generic solution if needed, instead of trying to be too generic to\n> > start with.\n> \n> And this whole class came from ControlValue\n> \n> >> +{\n> >> +public:\n> >> +\tValue();\n> >> +\tValue(bool value);\n> >> +\tValue(int value);\n> >> +\tValue(const char *value);\n> >> +\tValue(const std::string &value);\n> >> +\n> >> +\tValueType type() const { return type_; };\n> >> +\tbool isNull() const { return type_ == ValueNull; };\n> > \n> > isUnset() ?\n> \n> Well if it's not Null anymore, shouldn't it be isNone?\n> \n> Or do you prefer (Un)'set' to mirror the set() call.\n\nI'm fine with either, let's just avoid \"null\" and make the type name and\nfunction name consistent.\n\n> >> +\n> >> +\tvoid set(bool value);\n> >> +\tvoid set(int value);\n> >> +\tvoid set(int64_t value);\n> > \n> > This is asking for trouble, you will often end up with the wrong integer\n> > type without even noticing, unless you can remember and apply flawlessly\n> > the C++ integer types implicit conversions rules.\n> \n> Hrm... So types in the Function name?\n\nThere are multiple options. One of them is to treat all integer types as\nan int64_t internally, and have getInt() and getInt64() operate properly\non them. Another option would be to use explicit function names. I like\nthe former in theory, but it requires conversions in the getters, so it\nmay be a little bit less efficient. Simplicity of use or efficiency in\nthis case ? :-)\n\n> >> +\tvoid set(const char *value);\n> >> +\tvoid set(const std::string &value);\n> >> +\n> >> +\tbool getBool() const;\n> >> +\tint getInt() const;\n> >> +\tint getInt64() const;\n> > \n> > This should return a 64-bit integer.\n> \n> Indeed.\n> \n> >> +\tstd::string getString() const;\n> >> +\n> >> +\tstd::string toString() const;\n> >> +\n> >> +private:\n> >> +\tValueType type_;\n> >> +\n> >> +\tunion {\n> >> +\t\tbool bool_;\n> >> +\t\tint integer_;\n> >> +\t\tint64_t integer64_;\n> >> +\t};\n> >> +\tstd::string string_;\n> > \n> > As you've noticed, usage of an std::string makes it impossible to make\n> > the string_ member part of the value union, which reduces memory and CPU\n> > efficiency. Every integer value will need to construct an std::string.\n> > For this reason I would store the data for complex types as a pointer\n> > instead, and allocate it dynamically. It could be a pointer to a\n> > std::string, or just a const char *. The latter would likely be more\n> > efficient.\n> \n> Indeed. I think this is looking to get dropped rather than converted at\n> the moment.\n> \n> I wanted to make the Value class a generic re-usable class that could be\n> re-used in the existing Option parsers, and V4L2Controls, because we are\n> already repeating the same patterns across the code base.\n> \n> But Jacopo doesn't want to make use of it there either ...\n\nLet's model it to be efficient for its main use cases, and then we can\nextend it to cover additional use cases if possible without an\nefficiency impact. I quite like the idea of using it for both libcamera\nand V4L2 controls though, but that could be done later (and isn't a\nstrong requirement).\n\n> >> +};\n> >> +\n> >> +std::ostream &operator<<(std::ostream &stream, const Value &value);\n> >> +\n> >> +} /* namespace libcamera */\n> >> +\n> >> +#endif /* __LIBCAMERA_VALUE_H__ */\n> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >> index 68c7ce14b5b4..8e68373118df 100644\n> >> --- a/src/libcamera/meson.build\n> >> +++ b/src/libcamera/meson.build\n> >> @@ -27,6 +27,7 @@ libcamera_sources = files([\n> >> 'v4l2_device.cpp',\n> >> 'v4l2_subdevice.cpp',\n> >> 'v4l2_videodevice.cpp',\n> >> + 'value.cpp',\n> >> 'version.cpp',\n> >> ])\n> >> \n> >> diff --git a/src/libcamera/value.cpp b/src/libcamera/value.cpp\n> >> new file mode 100644\n> >> index 000000000000..f713907b38dd\n> >> --- /dev/null\n> >> +++ b/src/libcamera/value.cpp\n> >> @@ -0,0 +1,226 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2019, Google Inc.\n> >> + *\n> >> + * value.cpp - Abstract value handling\n> >> + */\n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \n> > \n> > Please move this as the first include to ensure that it compiles without\n> > depending on any implicitly included headers.\n> \n> Done.\n> \n> >> +\n> >> +#include \"log.h\" // For ASSERT()\n> > \n> > You don't need to keep the comment.\n> \n> Removed.\n> \n> >> +\n> >> +/**\n> >> + * \\file value.h\n> >> + * \\brief Handles variant value types as an abstract object\n> >> + */\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +/**\n> >> + * \\enum ValueType\n> >> + * Determines the type of value represented by a \\a Value\n> >> + * \\var ValueNull\n> >> + * Identifies an unset value\n> >> + * \\var ValueBool\n> >> + * Identifies controls storing a boolean value\n> >> + * \\var ValueInteger\n> >> + * Identifies controls storing an integer value\n> >> + * \\var ValueString\n> >> + * Identifies controls storing a string value\n> > \n> > You're missing the 64-bit integer.\n> \n> Added.\n> \n> >> + */\n> >> +\n> >> +/**\n> >> + * \\class Value\n> >> + * \\brief Abstract a value for a common data exchange\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Construct an empty Value.\n> >> + * The Value must be \\a set() before being used.\n> >> + */\n> >> +Value::Value()\n> >> +\t: type_(ValueNull)\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Construct a Boolean Value\n> >> + * \\param[in] value Boolean value to store\n> >> + */\n> >> +Value::Value(bool value)\n> >> +\t: type_(ValueBool), bool_(value)\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Construct an integer Value\n> >> + * \\param[in] value Integer value to store\n> >> + */\n> >> +Value::Value(int value)\n> >> +\t: type_(ValueInteger), integer_(value)\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Construct a string Value\n> >> + * \\param[in] value String representation to store\n> >> + */\n> >> +Value::Value(const char *value)\n> >> +\t: type_(ValueString), string_(value)\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Construct a string Value\n> >> + * \\param[in] value String representation to store\n> >> + */\n> >> +Value::Value(const std::string &value)\n> >> +\t: type_(ValueString), string_(value)\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\fn Value::type\n> >> + * \\brief Return the type of value represented by the object\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn Value::isNull\n> >> + * \\brief Determines if the Value is initialised\n> > \n> > s/Determines/Determine/\n> > \n> >> + * \\return true if the value type is ValueNull, false otherwise\n> > \n> > s/true/True/\n> > \n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Set the value with a boolean\n> >> + * \\param[in] value Boolean value to store\n> >> + */\n> >> +void Value::set(bool value)\n> >> +{\n> >> +\ttype_ = ValueBool;\n> >> +\tbool_ = value;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Set the value with an integer\n> >> + * \\param[in] value Integer value to store\n> >> + */\n> >> +void Value::set(int value)\n> >> +{\n> >> +\ttype_ = ValueInteger;\n> >> +\tinteger_ = value;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Set the value with a 64 bit integer\n> >> + * \\param[in] value Integer value to store\n> >> + */\n> >> +void Value::set(int64_t value)\n> >> +{\n> >> +\ttype_ = ValueInteger64;\n> >> +\tinteger64_ = value;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Set the value with a string representation\n> >> + * \\param[in] value String value to store\n> >> + */\n> >> +void Value::set(const char *value)\n> >> +{\n> >> +\ttype_ = ValueString;\n> >> +\tstring_ = value;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Set the value with a string representation\n> >> + * \\param[in] value String value to store\n> >> + */\n> >> +void Value::set(const std::string &value)\n> >> +{\n> >> +\ttype_ = ValueString;\n> >> +\tstring_ = value;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Get the boolean value.\n> >> + *\n> >> + * The Value type must be Boolean.\n> >> + */\n> >> +bool Value::getBool() const\n> >> +{\n> >> +\tASSERT(type_ == ValueBool);\n> > \n> > That's quite dangerous... Isn't there a large risk that the value will\n> > be set to a type and retrieved using a different but compatible type,\n> > and that the issue would only be caught at runtime, possibly in uncommon\n> > cases ? For instance consider the following code\n> \n> That's why I put the assert in - to catch those issues early?\n> \n> Warnings and Error prints will scroll off the logs...\n\nWith the condition being unlikely the ASSERT() may only trigger in\nproduction, which wouldn't be very nice.\n\n> > \n> > \tValue v;\n> > \n> > \tif (likely(condition))\n> > \t\tvalue.set(3);\n> > \telse\n> > \t\tvalue.set(0x100000000ULL);\n> > \n> > \t...\n> > \n> > \tint i = v.getInt();\n> > \n> > This will cause a crash in the unlikely event that condition is false,\n> > and could get unnoticed through testing. An explicit set API with type\n> > names may help there. Given that the Value class will be used for\n> > controls, I think we'll have to make sure we don't crash, invalid types\n> > will need to be caught and reported. We could also decide than an\n> > invalid type would result in the control taking a default value instead\n> > of crashing (but then this should be logged).\n> \n> I'm not sure taking a default value is appropriate either. I don't think\n> any 'default' would be correct.\n> \n> IMO - the types were important, and the asserts were to ensure strict\n> typing when used.\n\nCould we do it the Qt way, where the get functions return an invalid\nvalue if the type isn't convertible ? For integers and booleans there's\nno invalid values, so Qt adds an optional bool *ok argument. In practice\nI expect the callers to rarely use it though, so I think we need to pick\nbetween two error handling behaviours: returning a \"default\" value (0\nfor integers for instance) or crashing. What's best ? :-)\n\nNow that I wrote this, I realised that ASSERT() will be compiled out in\nrelease builds. Having it in debug builds is thus likely a good idea,\nbut we still need to decide on how to handle release builds.\n\n> If the set() call is changed to setInt() would this make you more\n> comfortable?\n\nA little bit :-)\n\n> >> +\n> >> +\treturn bool_;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Get the integer value.\n> >> + *\n> >> + * The Value type must be Integer.\n> >> + */\n> >> +int Value::getInt() const\n> >> +{\n> >> +\tASSERT(type_ == ValueInteger);\n> >> +\n> >> +\treturn integer_;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Get the 64bit integer value.\n> >> + *\n> >> + * The Value type must be Integer64.\n> >> + */\n> >> +int Value::getInt64() const\n> >> +{\n> >> +\tASSERT(type_ == ValueInteger64);\n> >> +\n> >> +\treturn integer64_;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Get the string value.\n> >> + *\n> >> + * The Value type must be String.\n> >> + */\n> >> +std::string Value::getString() const\n> >> +{\n> >> +\tASSERT(type_ == ValueString);\n> >> +\n> >> +\treturn string_;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Prepare a string representation of the Value\n> >> + */\n> >> +std::string Value::toString() const\n> >> +{\n> >> +\tswitch (type_) {\n> >> +\tcase ValueNull:\n> >> +\t\treturn \"\";\n> >> +\tcase ValueBool:\n> >> +\t\treturn bool_ ? \"True\" : \"False\";\n> >> +\tcase ValueInteger:\n> >> +\t\treturn std::to_string(integer_);\n> >> +\tcase ValueInteger64:\n> >> +\t\treturn std::to_string(integer64_);\n> >> +\tcase ValueString:\n> >> +\t\treturn string_;\n> >> +\t}\n> >> +\n> >> +\t/* Unreachable */\n> >> +\treturn \"\";\n> > \n> > Do you need this ? Does the compiler complain when the switch-case\n> > handles all the cases ?\n> \n> Yup.\n> \n> ../src/libcamera/value.cpp: In member function ‘std::__cxx11::string\n> libcamera::Value::toString() const’:\n> ../src/libcamera/value.cpp:212:1: error: control reaches end of non-void\n> function [-Werror=return-type]\n> }\n> ^\n> cc1plus: all warnings being treated as errors\n> \n> I can move it to a 'default:' statement which then removes the warning -\n> but should still be redundant/ unreachable.\n\nA default statement sounds nice, but I think that an error after the\nswitch() is better as in that case the compiler should warn you if you\nadd a new enum entry without adding a corresponding case.\n\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Provide a string stream representation of the Value \\a value to\n> >> + * the \\a stream.\n> > \n> > It's not a string stream, it's an output stream (and you should include\n> > ostream instead of sstream).\n> > \n> >> + */\n> >> +std::ostream &operator<<(std::ostream &stream, const Value &value)\n> >> +{\n> >> +\treturn stream << value.toString();\n> >> +}\n> >> +\n> >> +} /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 252EE600EB\n\tfor ;\n\tMon, 24 Jun 2019 14:11:39 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8B59A323;\n\tMon, 24 Jun 2019 14:11:38 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561378298;\n\tbh=gQ9WRTJRSXre0jMSBel8GGvHuAG1m2+/k4YVNhYZr00=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KhCGSrmur6UAKOh3xEhhfA+YAdZDF8QIlIkeTE3QRMFZS7+gMTuZBqxvNVIoFTqq0\n\taeQ/FHwl2v1D+YfQgKyjDKbbs4GR2rkNrsl85/V9awL+0zIlWCo5oA95DKzfhIz5/U\n\tam2YFNeIRBn0F+45YOHugGqzBL6w05CHTrM2FwOw=","Date":"Mon, 24 Jun 2019 15:09:08 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190624120908.GH5737@pendragon.ideasonboard.com>","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-2-kieran.bingham@ideasonboard.com>\n\t<20190622223943.GJ8156@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/9] libcamera: Value: Provide\n\tabstract value class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 12:11:39 -0000"}},{"id":2005,"web_url":"https://patchwork.libcamera.org/comment/2005/","msgid":"<3707934f-540e-c1c0-f360-16d1af1b3492@ideasonboard.com>","date":"2019-06-24T15:39:41","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/9] libcamera: Value: Provide\n\tabstract value class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 24/06/2019 13:09, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Mon, Jun 24, 2019 at 10:09:06AM +0100, Kieran Bingham wrote:\n>> On 22/06/2019 23:39, Laurent Pinchart wrote:\n>>> On Fri, Jun 21, 2019 at 05:13:53PM +0100, Kieran Bingham wrote:\n>>>> To facilitate passing typed data generically, implement a class which stores\n>>>> the type and value as efficiently as possible.\n>>>>\n>>>> Signed-off-by: Kieran Bingham \n>>>> ---\n>>>> include/libcamera/meson.build | 1 +\n>>>> include/libcamera/value.h | 63 ++++++++++\n>>>> src/libcamera/meson.build | 1 +\n>>>> src/libcamera/value.cpp | 226 ++++++++++++++++++++++++++++++++++\n>>>> 4 files changed, 291 insertions(+)\n>>>> create mode 100644 include/libcamera/value.h\n>>>> create mode 100644 src/libcamera/value.cpp\n>>>>\n>>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n>>>> index 201832105457..eb2211ae1fc3 100644\n>>>> --- a/include/libcamera/meson.build\n>>>> +++ b/include/libcamera/meson.build\n>>>> @@ -12,6 +12,7 @@ libcamera_api = files([\n>>>> 'signal.h',\n>>>> 'stream.h',\n>>>> 'timer.h',\n>>>> + 'value.h',\n>>>> 'version.h',\n>>>> ])\n>>>> \n>>>> diff --git a/include/libcamera/value.h b/include/libcamera/value.h\n>>>> new file mode 100644\n>>>> index 000000000000..00c5d53d5cc0\n>>>> --- /dev/null\n>>>> +++ b/include/libcamera/value.h\n>>>> @@ -0,0 +1,63 @@\n>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2019, Google Inc.\n>>>> + *\n>>>> + * value.h - Abstract value handling\n>>>> + */\n>>>> +\n>>>> +#include \n>>>> +\n>>>> +#ifndef __LIBCAMERA_VALUE_H__\n>>>> +#define __LIBCAMERA_VALUE_H__\n>>>> +\n>>>> +namespace libcamera {\n>>>> +\n>>>> +enum ValueType : uint8_t {\n>>>\n>>> Do we need an explicit uint8_t here ? It won't save memory as the next\n>>> member of the Value class will be aligned to a 64-bits boundary.\n>>\n>> True, but ValueType might be used in other locations too? Although then\n>> they'd only be used for checking the type - so storage isn't really an\n>> issue there either.\n>>\n>> My actual reason for playing around with specifying the types on enums\n>> was to explore the effects on the ABI.\n>>\n>> The ValueType enum should certainly never exceed 255 entries, which is\n>> one benefit of being explicitly smaller - but I do think on this\n>> occasion it is completely redundant so I'll just remove it now.\n>>\n>>>> +\tValueNull,\n>>>\n>>> Let's call it ValueUnset or ValueNone. Null has a well established\n>>> meaning.\n>>\n>> I started with ValueNone ... but moved to ValueNull ... but I'm happy to\n>> go back again :-)\n>>\n>>>> +\tValueBool,\n>>>> +\tValueInteger,\n>>>> +\tValueInteger64,\n>>>> +\tValueString,\n>>>> +};\n>>>> +\n>>>> +class Value\n>>>\n>>> I think I would call this ControlValue for now, and then expand it to a\n>>> more generic solution if needed, instead of trying to be too generic to\n>>> start with.\n>>\n>> And this whole class came from ControlValue\n>>\n>>>> +{\n>>>> +public:\n>>>> +\tValue();\n>>>> +\tValue(bool value);\n>>>> +\tValue(int value);\n>>>> +\tValue(const char *value);\n>>>> +\tValue(const std::string &value);\n>>>> +\n>>>> +\tValueType type() const { return type_; };\n>>>> +\tbool isNull() const { return type_ == ValueNull; };\n>>>\n>>> isUnset() ?\n>>\n>> Well if it's not Null anymore, shouldn't it be isNone?\n>>\n>> Or do you prefer (Un)'set' to mirror the set() call.\n> \n> I'm fine with either, let's just avoid \"null\" and make the type name and\n> function name consistent.\n> \n>>>> +\n>>>> +\tvoid set(bool value);\n>>>> +\tvoid set(int value);\n>>>> +\tvoid set(int64_t value);\n>>>\n>>> This is asking for trouble, you will often end up with the wrong integer\n>>> type without even noticing, unless you can remember and apply flawlessly\n>>> the C++ integer types implicit conversions rules.\n>>\n>> Hrm... So types in the Function name?\n> \n> There are multiple options. One of them is to treat all integer types as\n> an int64_t internally, and have getInt() and getInt64() operate properly\n> on them. Another option would be to use explicit function names. I like\n> the former in theory, but it requires conversions in the getters, so it\n> may be a little bit less efficient. Simplicity of use or efficiency in\n> this case ? :-)\n> \n>>>> +\tvoid set(const char *value);\n>>>> +\tvoid set(const std::string &value);\n>>>> +\n>>>> +\tbool getBool() const;\n>>>> +\tint getInt() const;\n>>>> +\tint getInt64() const;\n>>>\n>>> This should return a 64-bit integer.\n>>\n>> Indeed.\n>>\n>>>> +\tstd::string getString() const;\n>>>> +\n>>>> +\tstd::string toString() const;\n>>>> +\n>>>> +private:\n>>>> +\tValueType type_;\n>>>> +\n>>>> +\tunion {\n>>>> +\t\tbool bool_;\n>>>> +\t\tint integer_;\n>>>> +\t\tint64_t integer64_;\n>>>> +\t};\n>>>> +\tstd::string string_;\n>>>\n>>> As you've noticed, usage of an std::string makes it impossible to make\n>>> the string_ member part of the value union, which reduces memory and CPU\n>>> efficiency. Every integer value will need to construct an std::string.\n>>> For this reason I would store the data for complex types as a pointer\n>>> instead, and allocate it dynamically. It could be a pointer to a\n>>> std::string, or just a const char *. The latter would likely be more\n>>> efficient.\n>>\n>> Indeed. I think this is looking to get dropped rather than converted at\n>> the moment.\n>>\n>> I wanted to make the Value class a generic re-usable class that could be\n>> re-used in the existing Option parsers, and V4L2Controls, because we are\n>> already repeating the same patterns across the code base.\n>>\n>> But Jacopo doesn't want to make use of it there either ...\n> \n> Let's model it to be efficient for its main use cases, and then we can\n> extend it to cover additional use cases if possible without an\n> efficiency impact. I quite like the idea of using it for both libcamera\n> and V4L2 controls though, but that could be done later (and isn't a\n> strong requirement).\n\nYes, it can be done later, I don't want to postpone Jacopo's series.\n\nI'd still like to keep this called Value, do you want it all renamed to\nControlValue ?\n\n\n\n>>>> +};\n>>>> +\n>>>> +std::ostream &operator<<(std::ostream &stream, const Value &value);\n>>>> +\n>>>> +} /* namespace libcamera */\n>>>> +\n>>>> +#endif /* __LIBCAMERA_VALUE_H__ */\n>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>>>> index 68c7ce14b5b4..8e68373118df 100644\n>>>> --- a/src/libcamera/meson.build\n>>>> +++ b/src/libcamera/meson.build\n>>>> @@ -27,6 +27,7 @@ libcamera_sources = files([\n>>>> 'v4l2_device.cpp',\n>>>> 'v4l2_subdevice.cpp',\n>>>> 'v4l2_videodevice.cpp',\n>>>> + 'value.cpp',\n>>>> 'version.cpp',\n>>>> ])\n>>>> \n>>>> diff --git a/src/libcamera/value.cpp b/src/libcamera/value.cpp\n>>>> new file mode 100644\n>>>> index 000000000000..f713907b38dd\n>>>> --- /dev/null\n>>>> +++ b/src/libcamera/value.cpp\n>>>> @@ -0,0 +1,226 @@\n>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2019, Google Inc.\n>>>> + *\n>>>> + * value.cpp - Abstract value handling\n>>>> + */\n>>>> +\n>>>> +#include \n>>>> +#include \n>>>> +#include \n>>>> +\n>>>> +#include \n>>>\n>>> Please move this as the first include to ensure that it compiles without\n>>> depending on any implicitly included headers.\n>>\n>> Done.\n>>\n>>>> +\n>>>> +#include \"log.h\" // For ASSERT()\n>>>\n>>> You don't need to keep the comment.\n>>\n>> Removed.\n>>\n>>>> +\n>>>> +/**\n>>>> + * \\file value.h\n>>>> + * \\brief Handles variant value types as an abstract object\n>>>> + */\n>>>> +\n>>>> +namespace libcamera {\n>>>> +\n>>>> +/**\n>>>> + * \\enum ValueType\n>>>> + * Determines the type of value represented by a \\a Value\n>>>> + * \\var ValueNull\n>>>> + * Identifies an unset value\n>>>> + * \\var ValueBool\n>>>> + * Identifies controls storing a boolean value\n>>>> + * \\var ValueInteger\n>>>> + * Identifies controls storing an integer value\n>>>> + * \\var ValueString\n>>>> + * Identifies controls storing a string value\n>>>\n>>> You're missing the 64-bit integer.\n>>\n>> Added.\n>>\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\class Value\n>>>> + * \\brief Abstract a value for a common data exchange\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\brief Construct an empty Value.\n>>>> + * The Value must be \\a set() before being used.\n>>>> + */\n>>>> +Value::Value()\n>>>> +\t: type_(ValueNull)\n>>>> +{\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Construct a Boolean Value\n>>>> + * \\param[in] value Boolean value to store\n>>>> + */\n>>>> +Value::Value(bool value)\n>>>> +\t: type_(ValueBool), bool_(value)\n>>>> +{\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Construct an integer Value\n>>>> + * \\param[in] value Integer value to store\n>>>> + */\n>>>> +Value::Value(int value)\n>>>> +\t: type_(ValueInteger), integer_(value)\n>>>> +{\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Construct a string Value\n>>>> + * \\param[in] value String representation to store\n>>>> + */\n>>>> +Value::Value(const char *value)\n>>>> +\t: type_(ValueString), string_(value)\n>>>> +{\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Construct a string Value\n>>>> + * \\param[in] value String representation to store\n>>>> + */\n>>>> +Value::Value(const std::string &value)\n>>>> +\t: type_(ValueString), string_(value)\n>>>> +{\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\fn Value::type\n>>>> + * \\brief Return the type of value represented by the object\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn Value::isNull\n>>>> + * \\brief Determines if the Value is initialised\n>>>\n>>> s/Determines/Determine/\n>>>\n>>>> + * \\return true if the value type is ValueNull, false otherwise\n>>>\n>>> s/true/True/\n>>>\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\brief Set the value with a boolean\n>>>> + * \\param[in] value Boolean value to store\n>>>> + */\n>>>> +void Value::set(bool value)\n>>>> +{\n>>>> +\ttype_ = ValueBool;\n>>>> +\tbool_ = value;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Set the value with an integer\n>>>> + * \\param[in] value Integer value to store\n>>>> + */\n>>>> +void Value::set(int value)\n>>>> +{\n>>>> +\ttype_ = ValueInteger;\n>>>> +\tinteger_ = value;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Set the value with a 64 bit integer\n>>>> + * \\param[in] value Integer value to store\n>>>> + */\n>>>> +void Value::set(int64_t value)\n>>>> +{\n>>>> +\ttype_ = ValueInteger64;\n>>>> +\tinteger64_ = value;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Set the value with a string representation\n>>>> + * \\param[in] value String value to store\n>>>> + */\n>>>> +void Value::set(const char *value)\n>>>> +{\n>>>> +\ttype_ = ValueString;\n>>>> +\tstring_ = value;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Set the value with a string representation\n>>>> + * \\param[in] value String value to store\n>>>> + */\n>>>> +void Value::set(const std::string &value)\n>>>> +{\n>>>> +\ttype_ = ValueString;\n>>>> +\tstring_ = value;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Get the boolean value.\n>>>> + *\n>>>> + * The Value type must be Boolean.\n>>>> + */\n>>>> +bool Value::getBool() const\n>>>> +{\n>>>> +\tASSERT(type_ == ValueBool);\n>>>\n>>> That's quite dangerous... Isn't there a large risk that the value will\n>>> be set to a type and retrieved using a different but compatible type,\n>>> and that the issue would only be caught at runtime, possibly in uncommon\n>>> cases ? For instance consider the following code\n>>\n>> That's why I put the assert in - to catch those issues early?\n>>\n>> Warnings and Error prints will scroll off the logs...\n> \n> With the condition being unlikely the ASSERT() may only trigger in\n> production, which wouldn't be very nice.\n\n\nAm I misunderstanding /your/ definition of ASSERT? :\n\n> * \\def ASSERT(condition)\n> * \\brief Abort program execution if assertion fails\n> *\n> * If \\a condition is false, ASSERT() logs an error message with the Fatal log\n> * level and aborts program execution.\n> *\n> * If the macro NDEBUG is defined before including log.h, ASSERT() generates no\n> * code.\n\n\nASSERT should never fire in production, because NDEBUG should be set for\nBuildType=Release builds.\n\n\n\n>>>\n>>> \tValue v;\n>>>\n>>> \tif (likely(condition))\n>>> \t\tvalue.set(3);\n>>> \telse\n>>> \t\tvalue.set(0x100000000ULL);\n>>>\n>>> \t...\n>>>\n>>> \tint i = v.getInt();\n>>>\n>>> This will cause a crash in the unlikely event that condition is false,\n>>> and could get unnoticed through testing. An explicit set API with type\n>>> names may help there. Given that the Value class will be used for\n>>> controls, I think we'll have to make sure we don't crash, invalid types\n>>> will need to be caught and reported. We could also decide than an\n>>> invalid type would result in the control taking a default value instead\n>>> of crashing (but then this should be logged).\n>>\n>> I'm not sure taking a default value is appropriate either. I don't think\n>> any 'default' would be correct.\n>>\n>> IMO - the types were important, and the asserts were to ensure strict\n>> typing when used.\n> \n> Could we do it the Qt way, where the get functions return an invalid\n> value if the type isn't convertible ? For integers and booleans there's\n> no invalid values, so Qt adds an optional bool *ok argument. In practice\n\nEurgh ... an 'ok' bool sounds icky... I'll look at the Qt code ...\n\n\n> I expect the callers to rarely use it though, so I think we need to pick\n> between two error handling behaviours: returning a \"default\" value (0\n> for integers for instance) or crashing. What's best ? :-)\n\nCrashing in debug builds only :D and returning an undefined value otherwise.\n\n(it will be whatever the contents of the union is, with whatever\nimplicit type conversion occurs, which 'may' be as correct as it can be).\n\n> \n> Now that I wrote this, I realised that ASSERT() will be compiled out in\n> release builds. Having it in debug builds is thus likely a good idea,\n> but we still need to decide on how to handle release builds.\n\n\nAha, well I didn't need to paste that in above then :)\n\nToo late ! I'll leave it there :P\n\n> \n>> If the set() call is changed to setInt() would this make you more\n>> comfortable?\n> \n> A little bit :-)\n\nOk - I'll remove the implicit type sets.\n\n\n>>>> +\n>>>> +\treturn bool_;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Get the integer value.\n>>>> + *\n>>>> + * The Value type must be Integer.\n>>>> + */\n>>>> +int Value::getInt() const\n>>>> +{\n>>>> +\tASSERT(type_ == ValueInteger);\n>>>> +\n>>>> +\treturn integer_;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Get the 64bit integer value.\n>>>> + *\n>>>> + * The Value type must be Integer64.\n>>>> + */\n>>>> +int Value::getInt64() const\n>>>> +{\n>>>> +\tASSERT(type_ == ValueInteger64);\n>>>> +\n>>>> +\treturn integer64_;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Get the string value.\n>>>> + *\n>>>> + * The Value type must be String.\n>>>> + */\n>>>> +std::string Value::getString() const\n>>>> +{\n>>>> +\tASSERT(type_ == ValueString);\n>>>> +\n>>>> +\treturn string_;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Prepare a string representation of the Value\n>>>> + */\n>>>> +std::string Value::toString() const\n>>>> +{\n>>>> +\tswitch (type_) {\n>>>> +\tcase ValueNull:\n>>>> +\t\treturn \"\";\n>>>> +\tcase ValueBool:\n>>>> +\t\treturn bool_ ? \"True\" : \"False\";\n>>>> +\tcase ValueInteger:\n>>>> +\t\treturn std::to_string(integer_);\n>>>> +\tcase ValueInteger64:\n>>>> +\t\treturn std::to_string(integer64_);\n>>>> +\tcase ValueString:\n>>>> +\t\treturn string_;\n>>>> +\t}\n>>>> +\n>>>> +\t/* Unreachable */\n>>>> +\treturn \"\";\n>>>\n>>> Do you need this ? Does the compiler complain when the switch-case\n>>> handles all the cases ?\n>>\n>> Yup.\n>>\n>> ../src/libcamera/value.cpp: In member function ‘std::__cxx11::string\n>> libcamera::Value::toString() const’:\n>> ../src/libcamera/value.cpp:212:1: error: control reaches end of non-void\n>> function [-Werror=return-type]\n>> }\n>> ^\n>> cc1plus: all warnings being treated as errors\n>>\n>> I can move it to a 'default:' statement which then removes the warning -\n>> but should still be redundant/ unreachable.\n> \n> A default statement sounds nice, but I think that an error after the\n> switch() is better as in that case the compiler should warn you if you\n> add a new enum entry without adding a corresponding case.\n\nI agree, I'd rather the compiler tell us if we have a missing enum type.\n\n\n> \n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Provide a string stream representation of the Value \\a value to\n>>>> + * the \\a stream.\n>>>\n>>> It's not a string stream, it's an output stream (and you should include\n>>> ostream instead of sstream).\n>>>\n>>>> + */\n>>>> +std::ostream &operator<<(std::ostream &stream, const Value &value)\n>>>> +{\n>>>> +\treturn stream << value.toString();\n>>>> +}\n>>>> +\n>>>> +} /* namespace libcamera */\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A424061580\n\tfor ;\n\tMon, 24 Jun 2019 17:39:45 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 038AD323;\n\tMon, 24 Jun 2019 17:39:44 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561390785;\n\tbh=9A3UhLu/Cv5mi7y0ZrEHjnYaz1xj07RVQX4qw0nslGs=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=oFfYBqCzrFK0abn1K+tB8v6bO6zxlS0N4/CewZWRhYh9zpZARrRmz5wR5hHl1JF0W\n\tLgdUt6lVf1q9pwg9f4ZXYRwn3TXY6qwkWcyAha2hHHy1nOOFFOvuMGea4Tci5h466q\n\t0jh52bMwit0ZyALfs1LwBeDCGxZuSDIFu47R3vrg=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-2-kieran.bingham@ideasonboard.com>\n\t<20190622223943.GJ8156@pendragon.ideasonboard.com>\n\t\n\t<20190624120908.GH5737@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<3707934f-540e-c1c0-f360-16d1af1b3492@ideasonboard.com>","Date":"Mon, 24 Jun 2019 16:39:41 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190624120908.GH5737@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/9] libcamera: Value: Provide\n\tabstract value class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 15:39:45 -0000"}},{"id":2011,"web_url":"https://patchwork.libcamera.org/comment/2011/","msgid":"<20190624162545.GP5737@pendragon.ideasonboard.com>","date":"2019-06-24T16:25:45","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/9] libcamera: Value: Provide\n\tabstract value class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Jun 24, 2019 at 04:39:41PM +0100, Kieran Bingham wrote:\n> On 24/06/2019 13:09, Laurent Pinchart wrote:\n> > On Mon, Jun 24, 2019 at 10:09:06AM +0100, Kieran Bingham wrote:\n> >> On 22/06/2019 23:39, Laurent Pinchart wrote:\n> >>> On Fri, Jun 21, 2019 at 05:13:53PM +0100, Kieran Bingham wrote:\n> >>>> To facilitate passing typed data generically, implement a class which stores\n> >>>> the type and value as efficiently as possible.\n> >>>>\n> >>>> Signed-off-by: Kieran Bingham \n> >>>> ---\n> >>>> include/libcamera/meson.build | 1 +\n> >>>> include/libcamera/value.h | 63 ++++++++++\n> >>>> src/libcamera/meson.build | 1 +\n> >>>> src/libcamera/value.cpp | 226 ++++++++++++++++++++++++++++++++++\n> >>>> 4 files changed, 291 insertions(+)\n> >>>> create mode 100644 include/libcamera/value.h\n> >>>> create mode 100644 src/libcamera/value.cpp\n> >>>>\n> >>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> >>>> index 201832105457..eb2211ae1fc3 100644\n> >>>> --- a/include/libcamera/meson.build\n> >>>> +++ b/include/libcamera/meson.build\n> >>>> @@ -12,6 +12,7 @@ libcamera_api = files([\n> >>>> 'signal.h',\n> >>>> 'stream.h',\n> >>>> 'timer.h',\n> >>>> + 'value.h',\n> >>>> 'version.h',\n> >>>> ])\n> >>>> \n> >>>> diff --git a/include/libcamera/value.h b/include/libcamera/value.h\n> >>>> new file mode 100644\n> >>>> index 000000000000..00c5d53d5cc0\n> >>>> --- /dev/null\n> >>>> +++ b/include/libcamera/value.h\n> >>>> @@ -0,0 +1,63 @@\n> >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>>> +/*\n> >>>> + * Copyright (C) 2019, Google Inc.\n> >>>> + *\n> >>>> + * value.h - Abstract value handling\n> >>>> + */\n> >>>> +\n> >>>> +#include \n> >>>> +\n> >>>> +#ifndef __LIBCAMERA_VALUE_H__\n> >>>> +#define __LIBCAMERA_VALUE_H__\n> >>>> +\n> >>>> +namespace libcamera {\n> >>>> +\n> >>>> +enum ValueType : uint8_t {\n> >>>\n> >>> Do we need an explicit uint8_t here ? It won't save memory as the next\n> >>> member of the Value class will be aligned to a 64-bits boundary.\n> >>\n> >> True, but ValueType might be used in other locations too? Although then\n> >> they'd only be used for checking the type - so storage isn't really an\n> >> issue there either.\n> >>\n> >> My actual reason for playing around with specifying the types on enums\n> >> was to explore the effects on the ABI.\n> >>\n> >> The ValueType enum should certainly never exceed 255 entries, which is\n> >> one benefit of being explicitly smaller - but I do think on this\n> >> occasion it is completely redundant so I'll just remove it now.\n> >>\n> >>>> +\tValueNull,\n> >>>\n> >>> Let's call it ValueUnset or ValueNone. Null has a well established\n> >>> meaning.\n> >>\n> >> I started with ValueNone ... but moved to ValueNull ... but I'm happy to\n> >> go back again :-)\n> >>\n> >>>> +\tValueBool,\n> >>>> +\tValueInteger,\n> >>>> +\tValueInteger64,\n> >>>> +\tValueString,\n> >>>> +};\n> >>>> +\n> >>>> +class Value\n> >>>\n> >>> I think I would call this ControlValue for now, and then expand it to a\n> >>> more generic solution if needed, instead of trying to be too generic to\n> >>> start with.\n> >>\n> >> And this whole class came from ControlValue\n> >>\n> >>>> +{\n> >>>> +public:\n> >>>> +\tValue();\n> >>>> +\tValue(bool value);\n> >>>> +\tValue(int value);\n> >>>> +\tValue(const char *value);\n> >>>> +\tValue(const std::string &value);\n> >>>> +\n> >>>> +\tValueType type() const { return type_; };\n> >>>> +\tbool isNull() const { return type_ == ValueNull; };\n> >>>\n> >>> isUnset() ?\n> >>\n> >> Well if it's not Null anymore, shouldn't it be isNone?\n> >>\n> >> Or do you prefer (Un)'set' to mirror the set() call.\n> > \n> > I'm fine with either, let's just avoid \"null\" and make the type name and\n> > function name consistent.\n> > \n> >>>> +\n> >>>> +\tvoid set(bool value);\n> >>>> +\tvoid set(int value);\n> >>>> +\tvoid set(int64_t value);\n> >>>\n> >>> This is asking for trouble, you will often end up with the wrong integer\n> >>> type without even noticing, unless you can remember and apply flawlessly\n> >>> the C++ integer types implicit conversions rules.\n> >>\n> >> Hrm... So types in the Function name?\n> > \n> > There are multiple options. One of them is to treat all integer types as\n> > an int64_t internally, and have getInt() and getInt64() operate properly\n> > on them. Another option would be to use explicit function names. I like\n> > the former in theory, but it requires conversions in the getters, so it\n> > may be a little bit less efficient. Simplicity of use or efficiency in\n> > this case ? :-)\n> > \n> >>>> +\tvoid set(const char *value);\n> >>>> +\tvoid set(const std::string &value);\n> >>>> +\n> >>>> +\tbool getBool() const;\n> >>>> +\tint getInt() const;\n> >>>> +\tint getInt64() const;\n> >>>\n> >>> This should return a 64-bit integer.\n> >>\n> >> Indeed.\n> >>\n> >>>> +\tstd::string getString() const;\n> >>>> +\n> >>>> +\tstd::string toString() const;\n> >>>> +\n> >>>> +private:\n> >>>> +\tValueType type_;\n> >>>> +\n> >>>> +\tunion {\n> >>>> +\t\tbool bool_;\n> >>>> +\t\tint integer_;\n> >>>> +\t\tint64_t integer64_;\n> >>>> +\t};\n> >>>> +\tstd::string string_;\n> >>>\n> >>> As you've noticed, usage of an std::string makes it impossible to make\n> >>> the string_ member part of the value union, which reduces memory and CPU\n> >>> efficiency. Every integer value will need to construct an std::string.\n> >>> For this reason I would store the data for complex types as a pointer\n> >>> instead, and allocate it dynamically. It could be a pointer to a\n> >>> std::string, or just a const char *. The latter would likely be more\n> >>> efficient.\n> >>\n> >> Indeed. I think this is looking to get dropped rather than converted at\n> >> the moment.\n> >>\n> >> I wanted to make the Value class a generic re-usable class that could be\n> >> re-used in the existing Option parsers, and V4L2Controls, because we are\n> >> already repeating the same patterns across the code base.\n> >>\n> >> But Jacopo doesn't want to make use of it there either ...\n> > \n> > Let's model it to be efficient for its main use cases, and then we can\n> > extend it to cover additional use cases if possible without an\n> > efficiency impact. I quite like the idea of using it for both libcamera\n> > and V4L2 controls though, but that could be done later (and isn't a\n> > strong requirement).\n> \n> Yes, it can be done later, I don't want to postpone Jacopo's series.\n> \n> I'd still like to keep this called Value, do you want it all renamed to\n> ControlValue ?\n\nI have a slight preference for that, especially given that the\nimplementation is restricted to the types needed for controls, and that\nit tries to optimise for control-related use cases.\n\n> >>>> +};\n> >>>> +\n> >>>> +std::ostream &operator<<(std::ostream &stream, const Value &value);\n> >>>> +\n> >>>> +} /* namespace libcamera */\n> >>>> +\n> >>>> +#endif /* __LIBCAMERA_VALUE_H__ */\n> >>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >>>> index 68c7ce14b5b4..8e68373118df 100644\n> >>>> --- a/src/libcamera/meson.build\n> >>>> +++ b/src/libcamera/meson.build\n> >>>> @@ -27,6 +27,7 @@ libcamera_sources = files([\n> >>>> 'v4l2_device.cpp',\n> >>>> 'v4l2_subdevice.cpp',\n> >>>> 'v4l2_videodevice.cpp',\n> >>>> + 'value.cpp',\n> >>>> 'version.cpp',\n> >>>> ])\n> >>>> \n> >>>> diff --git a/src/libcamera/value.cpp b/src/libcamera/value.cpp\n> >>>> new file mode 100644\n> >>>> index 000000000000..f713907b38dd\n> >>>> --- /dev/null\n> >>>> +++ b/src/libcamera/value.cpp\n> >>>> @@ -0,0 +1,226 @@\n> >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>>> +/*\n> >>>> + * Copyright (C) 2019, Google Inc.\n> >>>> + *\n> >>>> + * value.cpp - Abstract value handling\n> >>>> + */\n> >>>> +\n> >>>> +#include \n> >>>> +#include \n> >>>> +#include \n> >>>> +\n> >>>> +#include \n> >>>\n> >>> Please move this as the first include to ensure that it compiles without\n> >>> depending on any implicitly included headers.\n> >>\n> >> Done.\n> >>\n> >>>> +\n> >>>> +#include \"log.h\" // For ASSERT()\n> >>>\n> >>> You don't need to keep the comment.\n> >>\n> >> Removed.\n> >>\n> >>>> +\n> >>>> +/**\n> >>>> + * \\file value.h\n> >>>> + * \\brief Handles variant value types as an abstract object\n> >>>> + */\n> >>>> +\n> >>>> +namespace libcamera {\n> >>>> +\n> >>>> +/**\n> >>>> + * \\enum ValueType\n> >>>> + * Determines the type of value represented by a \\a Value\n> >>>> + * \\var ValueNull\n> >>>> + * Identifies an unset value\n> >>>> + * \\var ValueBool\n> >>>> + * Identifies controls storing a boolean value\n> >>>> + * \\var ValueInteger\n> >>>> + * Identifies controls storing an integer value\n> >>>> + * \\var ValueString\n> >>>> + * Identifies controls storing a string value\n> >>>\n> >>> You're missing the 64-bit integer.\n> >>\n> >> Added.\n> >>\n> >>>> + */\n> >>>> +\n> >>>> +/**\n> >>>> + * \\class Value\n> >>>> + * \\brief Abstract a value for a common data exchange\n> >>>> + */\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Construct an empty Value.\n> >>>> + * The Value must be \\a set() before being used.\n> >>>> + */\n> >>>> +Value::Value()\n> >>>> +\t: type_(ValueNull)\n> >>>> +{\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Construct a Boolean Value\n> >>>> + * \\param[in] value Boolean value to store\n> >>>> + */\n> >>>> +Value::Value(bool value)\n> >>>> +\t: type_(ValueBool), bool_(value)\n> >>>> +{\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Construct an integer Value\n> >>>> + * \\param[in] value Integer value to store\n> >>>> + */\n> >>>> +Value::Value(int value)\n> >>>> +\t: type_(ValueInteger), integer_(value)\n> >>>> +{\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Construct a string Value\n> >>>> + * \\param[in] value String representation to store\n> >>>> + */\n> >>>> +Value::Value(const char *value)\n> >>>> +\t: type_(ValueString), string_(value)\n> >>>> +{\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Construct a string Value\n> >>>> + * \\param[in] value String representation to store\n> >>>> + */\n> >>>> +Value::Value(const std::string &value)\n> >>>> +\t: type_(ValueString), string_(value)\n> >>>> +{\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\fn Value::type\n> >>>> + * \\brief Return the type of value represented by the object\n> >>>> + */\n> >>>> +\n> >>>> +/**\n> >>>> + * \\fn Value::isNull\n> >>>> + * \\brief Determines if the Value is initialised\n> >>>\n> >>> s/Determines/Determine/\n> >>>\n> >>>> + * \\return true if the value type is ValueNull, false otherwise\n> >>>\n> >>> s/true/True/\n> >>>\n> >>>> + */\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Set the value with a boolean\n> >>>> + * \\param[in] value Boolean value to store\n> >>>> + */\n> >>>> +void Value::set(bool value)\n> >>>> +{\n> >>>> +\ttype_ = ValueBool;\n> >>>> +\tbool_ = value;\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Set the value with an integer\n> >>>> + * \\param[in] value Integer value to store\n> >>>> + */\n> >>>> +void Value::set(int value)\n> >>>> +{\n> >>>> +\ttype_ = ValueInteger;\n> >>>> +\tinteger_ = value;\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Set the value with a 64 bit integer\n> >>>> + * \\param[in] value Integer value to store\n> >>>> + */\n> >>>> +void Value::set(int64_t value)\n> >>>> +{\n> >>>> +\ttype_ = ValueInteger64;\n> >>>> +\tinteger64_ = value;\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Set the value with a string representation\n> >>>> + * \\param[in] value String value to store\n> >>>> + */\n> >>>> +void Value::set(const char *value)\n> >>>> +{\n> >>>> +\ttype_ = ValueString;\n> >>>> +\tstring_ = value;\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Set the value with a string representation\n> >>>> + * \\param[in] value String value to store\n> >>>> + */\n> >>>> +void Value::set(const std::string &value)\n> >>>> +{\n> >>>> +\ttype_ = ValueString;\n> >>>> +\tstring_ = value;\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Get the boolean value.\n> >>>> + *\n> >>>> + * The Value type must be Boolean.\n> >>>> + */\n> >>>> +bool Value::getBool() const\n> >>>> +{\n> >>>> +\tASSERT(type_ == ValueBool);\n> >>>\n> >>> That's quite dangerous... Isn't there a large risk that the value will\n> >>> be set to a type and retrieved using a different but compatible type,\n> >>> and that the issue would only be caught at runtime, possibly in uncommon\n> >>> cases ? For instance consider the following code\n> >>\n> >> That's why I put the assert in - to catch those issues early?\n> >>\n> >> Warnings and Error prints will scroll off the logs...\n> > \n> > With the condition being unlikely the ASSERT() may only trigger in\n> > production, which wouldn't be very nice.\n> \n> Am I misunderstanding /your/ definition of ASSERT? :\n> \n> > * \\def ASSERT(condition)\n> > * \\brief Abort program execution if assertion fails\n> > *\n> > * If \\a condition is false, ASSERT() logs an error message with the Fatal log\n> > * level and aborts program execution.\n> > *\n> > * If the macro NDEBUG is defined before including log.h, ASSERT() generates no\n> > * code.\n> \n> ASSERT should never fire in production, because NDEBUG should be set for\n> BuildType=Release builds.\n> \n> >>>\n> >>> \tValue v;\n> >>>\n> >>> \tif (likely(condition))\n> >>> \t\tvalue.set(3);\n> >>> \telse\n> >>> \t\tvalue.set(0x100000000ULL);\n> >>>\n> >>> \t...\n> >>>\n> >>> \tint i = v.getInt();\n> >>>\n> >>> This will cause a crash in the unlikely event that condition is false,\n> >>> and could get unnoticed through testing. An explicit set API with type\n> >>> names may help there. Given that the Value class will be used for\n> >>> controls, I think we'll have to make sure we don't crash, invalid types\n> >>> will need to be caught and reported. We could also decide than an\n> >>> invalid type would result in the control taking a default value instead\n> >>> of crashing (but then this should be logged).\n> >>\n> >> I'm not sure taking a default value is appropriate either. I don't think\n> >> any 'default' would be correct.\n> >>\n> >> IMO - the types were important, and the asserts were to ensure strict\n> >> typing when used.\n> > \n> > Could we do it the Qt way, where the get functions return an invalid\n> > value if the type isn't convertible ? For integers and booleans there's\n> > no invalid values, so Qt adds an optional bool *ok argument. In practice\n> \n> Eurgh ... an 'ok' bool sounds icky... I'll look at the Qt code ...\n\nYes, I don't like it much :-(\n\n> > I expect the callers to rarely use it though, so I think we need to pick\n> > between two error handling behaviours: returning a \"default\" value (0\n> > for integers for instance) or crashing. What's best ? :-)\n> \n> Crashing in debug builds only :D and returning an undefined value otherwise.\n> \n> (it will be whatever the contents of the union is, with whatever\n> implicit type conversion occurs, which 'may' be as correct as it can be).\n\nShould we return a fixed value instead, to make the behaviour\nreproducible ? Otherwise this may lead to bugs that are hard to\nreproduce.\n\n> > Now that I wrote this, I realised that ASSERT() will be compiled out in\n> > release builds. Having it in debug builds is thus likely a good idea,\n> > but we still need to decide on how to handle release builds.\n> \n> Aha, well I didn't need to paste that in above then :)\n> \n> Too late ! I'll leave it there :P\n> \n> >> If the set() call is changed to setInt() would this make you more\n> >> comfortable?\n> > \n> > A little bit :-)\n> \n> Ok - I'll remove the implicit type sets.\n\nIf we go for returning a fixed default value when the type doesn't\nmatch, and if we add conversion between compatible types, then\noverloading the set() function is fine.\n\n> >>>> +\n> >>>> +\treturn bool_;\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Get the integer value.\n> >>>> + *\n> >>>> + * The Value type must be Integer.\n> >>>> + */\n> >>>> +int Value::getInt() const\n> >>>> +{\n> >>>> +\tASSERT(type_ == ValueInteger);\n> >>>> +\n> >>>> +\treturn integer_;\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Get the 64bit integer value.\n> >>>> + *\n> >>>> + * The Value type must be Integer64.\n> >>>> + */\n> >>>> +int Value::getInt64() const\n> >>>> +{\n> >>>> +\tASSERT(type_ == ValueInteger64);\n> >>>> +\n> >>>> +\treturn integer64_;\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Get the string value.\n> >>>> + *\n> >>>> + * The Value type must be String.\n> >>>> + */\n> >>>> +std::string Value::getString() const\n> >>>> +{\n> >>>> +\tASSERT(type_ == ValueString);\n> >>>> +\n> >>>> +\treturn string_;\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Prepare a string representation of the Value\n> >>>> + */\n> >>>> +std::string Value::toString() const\n> >>>> +{\n> >>>> +\tswitch (type_) {\n> >>>> +\tcase ValueNull:\n> >>>> +\t\treturn \"\";\n> >>>> +\tcase ValueBool:\n> >>>> +\t\treturn bool_ ? \"True\" : \"False\";\n> >>>> +\tcase ValueInteger:\n> >>>> +\t\treturn std::to_string(integer_);\n> >>>> +\tcase ValueInteger64:\n> >>>> +\t\treturn std::to_string(integer64_);\n> >>>> +\tcase ValueString:\n> >>>> +\t\treturn string_;\n> >>>> +\t}\n> >>>> +\n> >>>> +\t/* Unreachable */\n> >>>> +\treturn \"\";\n> >>>\n> >>> Do you need this ? Does the compiler complain when the switch-case\n> >>> handles all the cases ?\n> >>\n> >> Yup.\n> >>\n> >> ../src/libcamera/value.cpp: In member function ‘std::__cxx11::string\n> >> libcamera::Value::toString() const’:\n> >> ../src/libcamera/value.cpp:212:1: error: control reaches end of non-void\n> >> function [-Werror=return-type]\n> >> }\n> >> ^\n> >> cc1plus: all warnings being treated as errors\n> >>\n> >> I can move it to a 'default:' statement which then removes the warning -\n> >> but should still be redundant/ unreachable.\n> > \n> > A default statement sounds nice, but I think that an error after the\n> > switch() is better as in that case the compiler should warn you if you\n> > add a new enum entry without adding a corresponding case.\n> \n> I agree, I'd rather the compiler tell us if we have a missing enum type.\n> \n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Provide a string stream representation of the Value \\a value to\n> >>>> + * the \\a stream.\n> >>>\n> >>> It's not a string stream, it's an output stream (and you should include\n> >>> ostream instead of sstream).\n> >>>\n> >>>> + */\n> >>>> +std::ostream &operator<<(std::ostream &stream, const Value &value)\n> >>>> +{\n> >>>> +\treturn stream << value.toString();\n> >>>> +}\n> >>>> +\n> >>>> +} /* namespace libcamera */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D567F61580\n\tfor ;\n\tMon, 24 Jun 2019 18:26:05 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 462212E7;\n\tMon, 24 Jun 2019 18:26:05 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561393565;\n\tbh=KrSKH9SW10enQr1cQPGy4hzkUpVt+e5NUpyoukLjFqE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=i0nlV/nhrn812qArRkrO51JQR0qIBXR8v4VKHd64r+SMPi5WzkGr/A9UQg0fd44Kb\n\tisKe/Muhgct5LmWULvDIaEG5HUwJkDzpImWyrfp3027BUkOK1QxfqvEvksmDRzpmfE\n\tDCmLTM/fkR81lmTeT7jpHRwhEgDo4db5o+4HCdP4=","Date":"Mon, 24 Jun 2019 19:25:45 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190624162545.GP5737@pendragon.ideasonboard.com>","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-2-kieran.bingham@ideasonboard.com>\n\t<20190622223943.GJ8156@pendragon.ideasonboard.com>\n\t\n\t<20190624120908.GH5737@pendragon.ideasonboard.com>\n\t<3707934f-540e-c1c0-f360-16d1af1b3492@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<3707934f-540e-c1c0-f360-16d1af1b3492@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/9] libcamera: Value: Provide\n\tabstract value class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 16:26:06 -0000"}}]