[{"id":1117,"web_url":"https://patchwork.libcamera.org/comment/1117/","msgid":"<20190323142808.GL4587@pendragon.ideasonboard.com>","date":"2019-03-23T14:28:08","subject":"Re: [libcamera-devel] [RFC 2/4] cam: options: Add an array data\n\ttype to OptionValue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Fri, Mar 22, 2019 at 02:53:47AM +0100, Niklas Söderlund wrote:\n> To allow specifying the same argument option multiple times a new type\n> of OptionValue is needed. As parsing of options is an iterative process\n> there is a need to append options as they are parsed so a more complex\n> constructor us needed which can combine already stored instances of an\n\ns/us needed/is needed/\n\n> option with new ones.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/cam/options.cpp | 21 +++++++++++++++++++++\n> src/cam/options.h | 6 ++++++\n> 2 files changed, 27 insertions(+)\n> \n> diff --git a/src/cam/options.cpp b/src/cam/options.cpp\n> index 497833397d894f82..7995a9b359764ec7 100644\n> --- a/src/cam/options.cpp\n> +++ b/src/cam/options.cpp\n> @@ -272,6 +272,14 @@ OptionValue::OptionValue(const KeyValueParser::Options &value)\n> {\n> }\n> \n> +OptionValue::OptionValue(const OptionValue &value,\n> +\t\t\t const std::vector &array)\n> +\t: type_(ValueArray)\n> +{\n> +\tarray_ = array;\n> +\tarray_.push_back(value);\n> +}\n> +\n> OptionValue::operator int() const\n> {\n> \treturn toInteger();\n> @@ -287,6 +295,11 @@ OptionValue::operator KeyValueParser::Options() const\n> \treturn toKeyValues();\n> }\n> \n> +OptionValue::operator std::vector() const\n> +{\n> +\treturn toArray();\n> +}\n> +\n> int OptionValue::toInteger() const\n> {\n> \tif (type_ != ValueInteger)\n> @@ -311,6 +324,14 @@ KeyValueParser::Options OptionValue::toKeyValues() const\n> \treturn keyValues_;\n> }\n> \n> +std::vector OptionValue::toArray() const\n> +{\n> +\tif (type_ != ValueArray)\n> +\t\treturn std::vector{};\n> +\n> +\treturn array_;\n> +}\n> +\n> /* -----------------------------------------------------------------------------\n> * OptionsParser\n> */\n> diff --git a/src/cam/options.h b/src/cam/options.h\n> index b33a90fc6058febf..789ba36187dd1fc3 100644\n> --- a/src/cam/options.h\n> +++ b/src/cam/options.h\n> @@ -10,6 +10,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> class KeyValueParser;\n> class OptionValue;\n> @@ -84,6 +85,7 @@ public:\n> \t\tValueInteger,\n> \t\tValueString,\n> \t\tValueKeyValue,\n> +\t\tValueArray,\n> \t};\n> \n> \tOptionValue();\n> @@ -91,22 +93,26 @@ public:\n> \tOptionValue(const char *value);\n> \tOptionValue(const std::string &value);\n> \tOptionValue(const KeyValueParser::Options &value);\n> +\tOptionValue(const OptionValue &value, const std::vector &array);\n\nThe patch looks fine except for this constructor. Isn't it possible for\nthe parser to create an OptionValue with an empty array the first time,\nand add new values to the array of the same OptionvAlue, instead of\nrecreating a new OptionValue every time ?\n\n> \n> \tValueType type() const { return type_; }\n> \n> \toperator int() const;\n> \toperator std::string() const;\n> \toperator KeyValueParser::Options() const;\n> +\toperator std::vector() const;\n> \n> \tint toInteger() const;\n> \tstd::string toString() const;\n> \tKeyValueParser::Options toKeyValues() const;\n> +\tstd::vector toArray() const;\n> \n> private:\n> \tValueType type_;\n> \tint integer_;\n> \tstd::string string_;\n> \tKeyValueParser::Options keyValues_;\n> +\tstd::vector array_;\n\nShould we group those four fields in a union ?\n\n> };\n> \n> class OptionsParser","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 62FA4610B6\n\tfor ;\n\tSat, 23 Mar 2019 15:28:21 +0100 (CET)","from pendragon.ideasonboard.com\n\t(p5269001-ipngn11702marunouchi.tokyo.ocn.ne.jp [114.158.195.1])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C237D22;\n\tSat, 23 Mar 2019 15:28:19 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1553351301;\n\tbh=VdZwYj+dWoRMmcr8gntP3g6pB5/GAkvCxpwDPmELqQc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sdcSQ4FTfSII4D4+LGxJcvtxU9nCkPoeRdNOTCXrl74Ff47p3Qne/5fI7y8YuPAcZ\n\tuEJDJQ9rmt9mGHvkuZzpA/+Qmf8u03D9zU9vV3dYQOcBRIDQ3wsKFvQypKvRhbzm8Z\n\tPoPozYMbOG+1JAXGdDO9qyWcPkfeN7vi0nFlCI8M=","Date":"Sat, 23 Mar 2019 16:28:08 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190323142808.GL4587@pendragon.ideasonboard.com>","References":"<20190322015349.14934-1-niklas.soderlund@ragnatech.se>\n\t<20190322015349.14934-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190322015349.14934-3-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 2/4] cam: options: Add an array data\n\ttype to OptionValue","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, 23 Mar 2019 14:28:21 -0000"}},{"id":1126,"web_url":"https://patchwork.libcamera.org/comment/1126/","msgid":"<20190325221854.GA9119@bigcity.dyn.berto.se>","date":"2019-03-25T22:18:54","subject":"Re: [libcamera-devel] [RFC 2/4] cam: options: Add an array data\n\ttype to OptionValue","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2019-03-23 16:28:08 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Fri, Mar 22, 2019 at 02:53:47AM +0100, Niklas Söderlund wrote:\n> > To allow specifying the same argument option multiple times a new type\n> > of OptionValue is needed. As parsing of options is an iterative process\n> > there is a need to append options as they are parsed so a more complex\n> > constructor us needed which can combine already stored instances of an\n> \n> s/us needed/is needed/\n> \n> > option with new ones.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/cam/options.cpp | 21 +++++++++++++++++++++\n> > src/cam/options.h | 6 ++++++\n> > 2 files changed, 27 insertions(+)\n> > \n> > diff --git a/src/cam/options.cpp b/src/cam/options.cpp\n> > index 497833397d894f82..7995a9b359764ec7 100644\n> > --- a/src/cam/options.cpp\n> > +++ b/src/cam/options.cpp\n> > @@ -272,6 +272,14 @@ OptionValue::OptionValue(const KeyValueParser::Options &value)\n> > {\n> > }\n> > \n> > +OptionValue::OptionValue(const OptionValue &value,\n> > +\t\t\t const std::vector &array)\n> > +\t: type_(ValueArray)\n> > +{\n> > +\tarray_ = array;\n> > +\tarray_.push_back(value);\n> > +}\n> > +\n> > OptionValue::operator int() const\n> > {\n> > \treturn toInteger();\n> > @@ -287,6 +295,11 @@ OptionValue::operator KeyValueParser::Options() const\n> > \treturn toKeyValues();\n> > }\n> > \n> > +OptionValue::operator std::vector() const\n> > +{\n> > +\treturn toArray();\n> > +}\n> > +\n> > int OptionValue::toInteger() const\n> > {\n> > \tif (type_ != ValueInteger)\n> > @@ -311,6 +324,14 @@ KeyValueParser::Options OptionValue::toKeyValues() const\n> > \treturn keyValues_;\n> > }\n> > \n> > +std::vector OptionValue::toArray() const\n> > +{\n> > +\tif (type_ != ValueArray)\n> > +\t\treturn std::vector{};\n> > +\n> > +\treturn array_;\n> > +}\n> > +\n> > /* -----------------------------------------------------------------------------\n> > * OptionsParser\n> > */\n> > diff --git a/src/cam/options.h b/src/cam/options.h\n> > index b33a90fc6058febf..789ba36187dd1fc3 100644\n> > --- a/src/cam/options.h\n> > +++ b/src/cam/options.h\n> > @@ -10,6 +10,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > \n> > class KeyValueParser;\n> > class OptionValue;\n> > @@ -84,6 +85,7 @@ public:\n> > \t\tValueInteger,\n> > \t\tValueString,\n> > \t\tValueKeyValue,\n> > +\t\tValueArray,\n> > \t};\n> > \n> > \tOptionValue();\n> > @@ -91,22 +93,26 @@ public:\n> > \tOptionValue(const char *value);\n> > \tOptionValue(const std::string &value);\n> > \tOptionValue(const KeyValueParser::Options &value);\n> > +\tOptionValue(const OptionValue &value, const std::vector &array);\n> \n> The patch looks fine except for this constructor. Isn't it possible for\n> the parser to create an OptionValue with an empty array the first time,\n> and add new values to the array of the same OptionvAlue, instead of\n> recreating a new OptionValue every time ?\n\n\nI tried both approaches and deemed both to have the same level of evil. \nHave no preference so will fix this and the comment in 3/4 in next \nversion.\n\n> \n> > \n> > \tValueType type() const { return type_; }\n> > \n> > \toperator int() const;\n> > \toperator std::string() const;\n> > \toperator KeyValueParser::Options() const;\n> > +\toperator std::vector() const;\n> > \n> > \tint toInteger() const;\n> > \tstd::string toString() const;\n> > \tKeyValueParser::Options toKeyValues() const;\n> > +\tstd::vector toArray() const;\n> > \n> > private:\n> > \tValueType type_;\n> > \tint integer_;\n> > \tstd::string string_;\n> > \tKeyValueParser::Options keyValues_;\n> > +\tstd::vector array_;\n> \n> Should we group those four fields in a union ?\n\nI tried this as well, but due to that the union will have members with \nnon-trivial constructors a custom constructor would be needed [1]. I \nthought this was too much work for the option parser to make it wort \nwhile. Will keep it as is for next version, if you disagree please let \nme know and we can fix it up on top.\n\n1. https://en.cppreference.com/w/cpp/language/union\n\n> \n> > };\n> > \n> > class OptionsParser\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 38E63600FD\n\tfor ;\n\tMon, 25 Mar 2019 23:18:57 +0100 (CET)","by mail-lf1-x142.google.com with SMTP id u9so7129091lfe.11\n\tfor ;\n\tMon, 25 Mar 2019 15:18:57 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tn5sm3590716lfh.6.2019.03.25.15.18.55\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 25 Mar 2019 15:18:55 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=b7PsZhumooEeOMvuSX7M/GMACBRTOz4lWq/e6YIXHOk=;\n\tb=X3sFCWMN8eSw/h5vonWxqRYbOH6IRu7syIC0gy0oLxt2zbxOLWtnsc9iEIHsjG3OjV\n\tPA8QPySuRBJfAWAA9ZOsNpYFkOG85Z2f/FL5ZwU9DaB559fxLErC3xER10kk9sFidyAz\n\tPdj3UirvAlCDOY8XwwC+k8w7cKJWt9+4KLjAinTk64wsyD+VlTr3QnYu+Id9zCgaIord\n\tB1QkyHPWnfgMa6K03wNI3nno8kaffQI4ENIn1u1SJVSTbeLyoIGVsP+feyQ7lYRvw+g0\n\tsmam1ofocd2/w92tvmtyzv2Gg0EQMWzK5h7RHzYcgG99YPhsT0ifDZ4i/HyXUt9js2RL\n\tFEuw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=b7PsZhumooEeOMvuSX7M/GMACBRTOz4lWq/e6YIXHOk=;\n\tb=oE/D2uD4WZdrxP70mBQbXFrGOL5LVhPjMD5kNDRrdv88Tpb4SbhMiFfFovssu/otMV\n\tIPOssJM790MU+myiCGM89aJhapSiSrkBbT9FiUl47Q9xn1Mj07IVq5VFDJiIBety1Lu2\n\tnxjXjKNxZhUfUrgi5dOLKnHcNxbKlUHRQp6bWK7j86B76ecrHexYBIqaL1jNS56uK+qR\n\t2QJmeQpVamxqABG83brEKp31nr8q4WAcItT/RmTRoMzi1PW56PihPXzGMGgktZEF+DYi\n\ti1rzIr74UWn8a9WYP1j+P9RdE0TQNN5aKdtXBXMKpnWYPqHD3uIoTyZrOPSkBbeYBDcS\n\t/Ovw==","X-Gm-Message-State":"APjAAAVXA85gOeGJpSlSzaaVNfDDb2Y0GKk97vvb+fValOGQeRDaSrxb\n\tJmrATag5KiMC1zbCRGVHR9FvUg==","X-Google-Smtp-Source":"APXvYqw0cdq3Do/YW7jmU9lgu6UpbKSltPeY28S+j/zYjpTwWbIKypam/DDMmBGneLGpcgKzlp1LgA==","X-Received":"by 2002:ac2:489b:: with SMTP id\n\tx27mr14283925lfc.107.1553552336372; \n\tMon, 25 Mar 2019 15:18:56 -0700 (PDT)","Date":"Mon, 25 Mar 2019 23:18:54 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190325221854.GA9119@bigcity.dyn.berto.se>","References":"<20190322015349.14934-1-niklas.soderlund@ragnatech.se>\n\t<20190322015349.14934-3-niklas.soderlund@ragnatech.se>\n\t<20190323142808.GL4587@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190323142808.GL4587@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [RFC 2/4] cam: options: Add an array data\n\ttype to OptionValue","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, 25 Mar 2019 22:18:57 -0000"}}]