[{"id":79,"web_url":"https://patchwork.libcamera.org/comment/79/","msgid":"<20181224104216.GC2364@archlinux>","date":"2018-12-24T10:42:16","subject":"Re: [libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator:\n\tadd DeviceInfo class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Sun, Dec 23, 2018 at 12:00:32AM +0100, Niklas S??derlund wrote:\n> Provide a DeviceInfo class which holds all information from the initial\n> enumeration of a media device. Not all information available at a media\n> device is stored, only the information needed for a pipeline handler to\n> find a specific device.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/deviceenumerator.cpp | 78 ++++++++++++++++++++++++\n> src/libcamera/include/deviceenumerator.h | 44 +++++++++++++\n> src/libcamera/meson.build | 2 +\n> 3 files changed, 124 insertions(+)\n> create mode 100644 src/libcamera/deviceenumerator.cpp\n> create mode 100644 src/libcamera/include/deviceenumerator.h\n>\n> diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp\n> new file mode 100644\n> index 0000000000000000..7c44a65b45472ef3\n> --- /dev/null\n> +++ b/src/libcamera/deviceenumerator.cpp\n> @@ -0,0 +1,78 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * deviceenumerator.cpp - Enumeration and matching\n> + */\n> +\n> +#include \"deviceenumerator.h\"\n> +#include \"log.h\"\n> +\n> +namespace libcamera {\n> +\n> +/* -----------------------------------------------------------------------------\n> + * DeviceInfo\n> + */\n> +\n> +DeviceInfo::DeviceInfo(const std::string &devnode, const struct media_device_info &info,\n> +\t\t const std::map &entities)\n> +\t: acquired_(false), devnode_(devnode), info_(info), entities_(entities)\n> +{\n> +\tfor (const auto &entity : entities_)\n> +\t\tLOG(Info) << \"Device: \" << devnode_ << \" Entity: '\" << entity.first << \"' -> \" << entity.second;\n> +}\n> +\n> +int DeviceInfo::acquire()\n> +{\n> +\tif (acquired_)\n> +\t\treturn -EBUSY;\n> +\n> +\tacquired_ = true;\n> +\n> +\treturn 0;\n> +}\n> +\n> +void DeviceInfo::release()\n> +{\n> +\tacquired_ = false;\n> +}\n> +\n> +bool DeviceInfo::busy() const\n> +{\n> +\treturn acquired_;\n> +}\n> +\n> +const std::string &DeviceInfo::devnode() const\n> +{\n> +\treturn devnode_;\n> +}\n> +\n> +const struct media_device_info &DeviceInfo::info() const\n> +{\n> +\treturn info_;\n> +}\n> +\n> +std::vector DeviceInfo::entities() const\n> +{\n> +\tstd::vector entities;\n> +\n> +\tfor (const auto &entity : entities_)\n> +\t\tentities.push_back(entity.first);\n> +\n> +\treturn entities;\n\nAre you returning (by copy) a variable with local scope?\nWhy coldn't you just return (as const reference maybe) entities_ ?\n\n> +}\n> +\n> +bool DeviceInfo::lookup(const std::string &name, std::string &devnode) const\n> +{\n> +\tauto it = entities_.find(name);\n> +\n> +\tif (it == entities_.end()) {\n> +\t\tLOG(Error) << \"Trying to lookup entity '\" << name << \"' which do not exist\";\n\nnit: \"does not exist\"\n\n> +\t\treturn false;\n> +\t}\n> +\n> +\tdevnode = it->second;\n> +\treturn true;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h\n> new file mode 100644\n> index 0000000000000000..0136ed6ea23bf65e\n> --- /dev/null\n> +++ b/src/libcamera/include/deviceenumerator.h\n> @@ -0,0 +1,44 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * deviceenumerator.h - API to enumerate and find media devices\n> + */\n> +#ifndef __LIBCAMERA_DEVICEENUMERATE_H__\n> +#define __LIBCAMERA_DEVICEENUMERATE_H__\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class DeviceInfo\n> +{\n> +public:\n> +\tDeviceInfo(const std::string &devnode, const struct media_device_info &info,\n> +\t\t const std::map &entities);\n> +\n> +\tint acquire();\n> +\tvoid release();\n> +\tbool busy() const;\n\nTrivial methods might be inlined here.\n\n> +\n> +\tconst std::string &devnode() const;\n> +\tconst struct media_device_info &info() const;\n> +\tstd::vector entities() const;\n> +\n> +\tbool lookup(const std::string &name, std::string &devnode) const;\n> +\n> +private:\n> +\tbool acquired_;\n> +\n> +\tstd::string devnode_;\n> +\tstruct media_device_info info_;\n> +\tstd::map entities_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 52b556a8ed4050cb..17cdf06dd2bedfb3 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -1,10 +1,12 @@\n> libcamera_sources = files([\n> 'camera.cpp',\n> + 'deviceenumerator.cpp',\n> 'log.cpp',\n> 'main.cpp',\n> ])\n>\n> libcamera_headers = files([\n> + 'include/deviceenumerator.h',\n> 'include/log.h',\n> 'include/utils.h',\n> ])\n> --\n> 2.20.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2108060B0B\n\tfor ;\n\tMon, 24 Dec 2018 11:42:19 +0100 (CET)","from archlinux (host54-51-dynamic.16-87-r.retail.telecomitalia.it\n\t[87.16.51.54]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 36505E0004;\n\tMon, 24 Dec 2018 10:42:17 +0000 (UTC)"],"X-Originating-IP":"87.16.51.54","Date":"Mon, 24 Dec 2018 11:42:16 +0100","From":"Jacopo Mondi ","To":"Niklas S??derlund ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181224104216.GC2364@archlinux>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-4-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"DIOMP1UsTsWJauNi\"","Content-Disposition":"inline","In-Reply-To":"<20181222230041.29999-4-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.11.1 (2018-12-01)","Subject":"Re: [libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator:\n\tadd DeviceInfo 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 Dec 2018 10:42:19 -0000"}},{"id":86,"web_url":"https://patchwork.libcamera.org/comment/86/","msgid":"<20181228030657.GP19796@bigcity.dyn.berto.se>","date":"2018-12-28T03:06:57","subject":"Re: [libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator:\n\tadd DeviceInfo class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your feedback.\n\nOn 2018-12-24 11:42:16 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Sun, Dec 23, 2018 at 12:00:32AM +0100, Niklas S??derlund wrote:\n> > Provide a DeviceInfo class which holds all information from the initial\n> > enumeration of a media device. Not all information available at a media\n> > device is stored, only the information needed for a pipeline handler to\n> > find a specific device.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/deviceenumerator.cpp | 78 ++++++++++++++++++++++++\n> > src/libcamera/include/deviceenumerator.h | 44 +++++++++++++\n> > src/libcamera/meson.build | 2 +\n> > 3 files changed, 124 insertions(+)\n> > create mode 100644 src/libcamera/deviceenumerator.cpp\n> > create mode 100644 src/libcamera/include/deviceenumerator.h\n> >\n> > diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp\n> > new file mode 100644\n> > index 0000000000000000..7c44a65b45472ef3\n> > --- /dev/null\n> > +++ b/src/libcamera/deviceenumerator.cpp\n> > @@ -0,0 +1,78 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * deviceenumerator.cpp - Enumeration and matching\n> > + */\n> > +\n> > +#include \"deviceenumerator.h\"\n> > +#include \"log.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +/* -----------------------------------------------------------------------------\n> > + * DeviceInfo\n> > + */\n> > +\n> > +DeviceInfo::DeviceInfo(const std::string &devnode, const struct media_device_info &info,\n> > +\t\t const std::map &entities)\n> > +\t: acquired_(false), devnode_(devnode), info_(info), entities_(entities)\n> > +{\n> > +\tfor (const auto &entity : entities_)\n> > +\t\tLOG(Info) << \"Device: \" << devnode_ << \" Entity: '\" << entity.first << \"' -> \" << entity.second;\n> > +}\n> > +\n> > +int DeviceInfo::acquire()\n> > +{\n> > +\tif (acquired_)\n> > +\t\treturn -EBUSY;\n> > +\n> > +\tacquired_ = true;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void DeviceInfo::release()\n> > +{\n> > +\tacquired_ = false;\n> > +}\n> > +\n> > +bool DeviceInfo::busy() const\n> > +{\n> > +\treturn acquired_;\n> > +}\n> > +\n> > +const std::string &DeviceInfo::devnode() const\n> > +{\n> > +\treturn devnode_;\n> > +}\n> > +\n> > +const struct media_device_info &DeviceInfo::info() const\n> > +{\n> > +\treturn info_;\n> > +}\n> > +\n> > +std::vector DeviceInfo::entities() const\n> > +{\n> > +\tstd::vector entities;\n> > +\n> > +\tfor (const auto &entity : entities_)\n> > +\t\tentities.push_back(entity.first);\n> > +\n> > +\treturn entities;\n> \n> Are you returning (by copy) a variable with local scope?\n> Why coldn't you just return (as const reference maybe) entities_ ?\n\nThis is by design. The idea is that a new copy of the entities in a \nvector instead of the class internal object of the model of entities as \nthe use-case here is to do matching. Why would it be beneficial to \nreturn a internal data structure?\n\n> \n> > +}\n> > +\n> > +bool DeviceInfo::lookup(const std::string &name, std::string &devnode) const\n> > +{\n> > +\tauto it = entities_.find(name);\n> > +\n> > +\tif (it == entities_.end()) {\n> > +\t\tLOG(Error) << \"Trying to lookup entity '\" << name << \"' which do not exist\";\n> \n> nit: \"does not exist\"\n\nMy comprehension of the Queens English is lacking so I trust you on \nthis. For my education is not 'does' used when there is more then one \nwhile 'do' is used when there is exactly one?\n\n> \n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tdevnode = it->second;\n> > +\treturn true;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h\n> > new file mode 100644\n> > index 0000000000000000..0136ed6ea23bf65e\n> > --- /dev/null\n> > +++ b/src/libcamera/include/deviceenumerator.h\n> > @@ -0,0 +1,44 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * deviceenumerator.h - API to enumerate and find media devices\n> > + */\n> > +#ifndef __LIBCAMERA_DEVICEENUMERATE_H__\n> > +#define __LIBCAMERA_DEVICEENUMERATE_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class DeviceInfo\n> > +{\n> > +public:\n> > +\tDeviceInfo(const std::string &devnode, const struct media_device_info &info,\n> > +\t\t const std::map &entities);\n> > +\n> > +\tint acquire();\n> > +\tvoid release();\n> > +\tbool busy() const;\n> \n> Trivial methods might be inlined here.\n\nThere are other methods who can't be inlined for this class. Why spread \nthe implementation around?\n\n> \n> > +\n> > +\tconst std::string &devnode() const;\n> > +\tconst struct media_device_info &info() const;\n> > +\tstd::vector entities() const;\n> > +\n> > +\tbool lookup(const std::string &name, std::string &devnode) const;\n> > +\n> > +private:\n> > +\tbool acquired_;\n> > +\n> > +\tstd::string devnode_;\n> > +\tstruct media_device_info info_;\n> > +\tstd::map entities_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 52b556a8ed4050cb..17cdf06dd2bedfb3 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -1,10 +1,12 @@\n> > libcamera_sources = files([\n> > 'camera.cpp',\n> > + 'deviceenumerator.cpp',\n> > 'log.cpp',\n> > 'main.cpp',\n> > ])\n> >\n> > libcamera_headers = files([\n> > + 'include/deviceenumerator.h',\n> > 'include/log.h',\n> > 'include/utils.h',\n> > ])\n> > --\n> > 2.20.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1803A600CC\n\tfor ;\n\tFri, 28 Dec 2018 04:06:59 +0100 (CET)","by mail-lf1-x143.google.com with SMTP id a16so13786085lfg.3\n\tfor ;\n\tThu, 27 Dec 2018 19:06:59 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\t12-v6sm8247105ljf.96.2018.12.27.19.06.57\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 27 Dec 2018 19:06:57 -0800 (PST)"],"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=4dCG/0qa39qszzQNGLgwH2F9exC2pph30JdlNAalP+8=;\n\tb=DGkQVir0us6rIV77E2H4MDKTfieaPz1tSS2aFYUax8CdLO5bSyRohcyo54TJ/TQINe\n\tDQZ5hCGhWbSRTploIs7nhau3m2d3rbR9TOZNywnlZMf5eCpDo9/k2wTZHI2FaZo/+d4m\n\tD7knPsD3YZ9eTcodHN6A6ynF5nHqaiP0Jqj6rFYgzGOH5pWFDzPpKZv+5Ji4L95awdOR\n\tN2pV7H8cdvN2z9Mu+/CazI+vV5t8bMnwwJ21dMcEvZKyhx+7mchJO836bbnBwT5RDCR5\n\tAIgqypa2HXy/CXVMO/AbsTrWR3mssGk9FfnsMcd/RtFOu/Gbpx+Ea5MZsKdnpSHtskiA\n\tR51w==","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=4dCG/0qa39qszzQNGLgwH2F9exC2pph30JdlNAalP+8=;\n\tb=EDpIj38tIlClRlzL2GHUUSe0uXIIZKDXSCYfVLXc9/HHpjaHx5uOCV018JZIviWA5M\n\tG6FvQkj6FNd8zdg3mWYoDd0+FNexeDnERrUf/VkepgfOEoZ3Mc7eGcKvCTlQNeDo8Aab\n\tNRqrPbw7kCxw9XpRvrJUo+qWdDVJq39ijcIqBKpNYZMUbzv4/cfdTmuRCk0iLT3crfbW\n\tF5Wt6+BsvzTMFrxWIvQD/oJS+JixCIWBW+d7P13DuoS4fZfKaFEwM3IqH7orT0rAM2mj\n\tYoeDw+ApPqizkdm82dvQlk/P8KJbx1NImR9+JF9W+0TOtXRwmbqOs/YVejvspXuhRw1g\n\t3cDw==","X-Gm-Message-State":"AA+aEWZQEy1v9VDYlake1DYotFAur/f33N9IRYZflCLPoDRiOEduQIIn\n\tBNUI5v7/DQqZRmzhEKUepvUlvA==","X-Google-Smtp-Source":"AFSGD/UuJzHLNf3OpfnxWvJYqPzCyYdKumPm2lMEv64E1Vz6Jftq8ZEUl0Vmpe3l5EN8SpYQXnffwg==","X-Received":"by 2002:a19:9b50:: with SMTP id\n\td77mr13431551lfe.137.1545966418293; \n\tThu, 27 Dec 2018 19:06:58 -0800 (PST)","Date":"Fri, 28 Dec 2018 04:06:57 +0100","From":"Niklas S??derlund ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181228030657.GP19796@bigcity.dyn.berto.se>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-4-niklas.soderlund@ragnatech.se>\n\t<20181224104216.GC2364@archlinux>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20181224104216.GC2364@archlinux>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator:\n\tadd DeviceInfo 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":"Fri, 28 Dec 2018 03:06:59 -0000"}},{"id":92,"web_url":"https://patchwork.libcamera.org/comment/92/","msgid":"<20181228082148.GC909@uno.localdomain>","date":"2018-12-28T08:21:48","subject":"Re: [libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator:\n\tadd DeviceInfo class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n just replying to a few things\n\nOn Fri, Dec 28, 2018 at 04:06:57AM +0100, Niklas S??derlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your feedback.\n>\n> On 2018-12-24 11:42:16 +0100, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Sun, Dec 23, 2018 at 12:00:32AM +0100, Niklas S??derlund wrote:\n> > > Provide a DeviceInfo class which holds all information from the initial\n> > > enumeration of a media device. Not all information available at a media\n> > > device is stored, only the information needed for a pipeline handler to\n> > > find a specific device.\n> > >\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/libcamera/deviceenumerator.cpp | 78 ++++++++++++++++++++++++\n> > > src/libcamera/include/deviceenumerator.h | 44 +++++++++++++\n> > > src/libcamera/meson.build | 2 +\n> > > 3 files changed, 124 insertions(+)\n> > > create mode 100644 src/libcamera/deviceenumerator.cpp\n> > > create mode 100644 src/libcamera/include/deviceenumerator.h\n> > >\n> > > diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp\n> > > new file mode 100644\n> > > index 0000000000000000..7c44a65b45472ef3\n> > > --- /dev/null\n> > > +++ b/src/libcamera/deviceenumerator.cpp\n> > > @@ -0,0 +1,78 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2018, Google Inc.\n> > > + *\n> > > + * deviceenumerator.cpp - Enumeration and matching\n> > > + */\n> > > +\n> > > +#include \"deviceenumerator.h\"\n> > > +#include \"log.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +/* -----------------------------------------------------------------------------\n> > > + * DeviceInfo\n> > > + */\n> > > +\n> > > +DeviceInfo::DeviceInfo(const std::string &devnode, const struct media_device_info &info,\n> > > +\t\t const std::map &entities)\n> > > +\t: acquired_(false), devnode_(devnode), info_(info), entities_(entities)\n> > > +{\n> > > +\tfor (const auto &entity : entities_)\n> > > +\t\tLOG(Info) << \"Device: \" << devnode_ << \" Entity: '\" << entity.first << \"' -> \" << entity.second;\n> > > +}\n> > > +\n> > > +int DeviceInfo::acquire()\n> > > +{\n> > > +\tif (acquired_)\n> > > +\t\treturn -EBUSY;\n> > > +\n> > > +\tacquired_ = true;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +void DeviceInfo::release()\n> > > +{\n> > > +\tacquired_ = false;\n> > > +}\n> > > +\n> > > +bool DeviceInfo::busy() const\n> > > +{\n> > > +\treturn acquired_;\n> > > +}\n> > > +\n> > > +const std::string &DeviceInfo::devnode() const\n> > > +{\n> > > +\treturn devnode_;\n> > > +}\n> > > +\n> > > +const struct media_device_info &DeviceInfo::info() const\n> > > +{\n> > > +\treturn info_;\n> > > +}\n> > > +\n> > > +std::vector DeviceInfo::entities() const\n> > > +{\n> > > +\tstd::vector entities;\n> > > +\n> > > +\tfor (const auto &entity : entities_)\n> > > +\t\tentities.push_back(entity.first);\n> > > +\n> > > +\treturn entities;\n> >\n> > Are you returning (by copy) a variable with local scope?\n> > Why coldn't you just return (as const reference maybe) entities_ ?\n>\n> This is by design. The idea is that a new copy of the entities in a\n> vector instead of the class internal object of the model of entities as\n> the use-case here is to do matching. Why would it be beneficial to\n> return a internal data structure?\n>\n\nIt would avoid moving around a vector by copy. You only use\nthe returned vector to compare entities names, so you could just\nsimply return a const reference to your internal data structure.\n\nI would actually ask why would it be beneficial to copy all entities\nnames in a new vector and copy it back, if that's not going to be\nmodified anyhow. Furthermore, you could have used operator= to copy\nthe internal one: http://www.cplusplus.com/reference/vector/vector/operator=/\n\n> >\n> > > +}\n> > > +\n> > > +bool DeviceInfo::lookup(const std::string &name, std::string &devnode) const\n> > > +{\n> > > +\tauto it = entities_.find(name);\n> > > +\n> > > +\tif (it == entities_.end()) {\n> > > +\t\tLOG(Error) << \"Trying to lookup entity '\" << name << \"' which do not exist\";\n> >\n> > nit: \"does not exist\"\n>\n> My comprehension of the Queens English is lacking so I trust you on\n\nDon't!\n\n> this. For my education is not 'does' used when there is more then one\n> while 'do' is used when there is exactly one?\n>\n\nYou might be right, let's wait for some more educated (or native)\nspeaker to school us.\n\n> >\n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\tdevnode = it->second;\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h\n> > > new file mode 100644\n> > > index 0000000000000000..0136ed6ea23bf65e\n> > > --- /dev/null\n> > > +++ b/src/libcamera/include/deviceenumerator.h\n> > > @@ -0,0 +1,44 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2018, Google Inc.\n> > > + *\n> > > + * deviceenumerator.h - API to enumerate and find media devices\n> > > + */\n> > > +#ifndef __LIBCAMERA_DEVICEENUMERATE_H__\n> > > +#define __LIBCAMERA_DEVICEENUMERATE_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class DeviceInfo\n> > > +{\n> > > +public:\n> > > +\tDeviceInfo(const std::string &devnode, const struct media_device_info &info,\n> > > +\t\t const std::map &entities);\n> > > +\n> > > +\tint acquire();\n> > > +\tvoid release();\n> > > +\tbool busy() const;\n> >\n> > Trivial methods might be inlined here.\n>\n> There are other methods who can't be inlined for this class. Why spread\n> the implementation around?\n>\n\nI'm in favour of inlining single line and trivial methods/constructors as\nmuch as we could. It reduce the size of the cpp file, and makes clear\nfrom reading the header the method is trivial. I'm not much concerned about\nspreading per se, as we all navigate tags don't we?\n\n(let alone the discussion about the fact inlining in class declaration\nis a hint for the compiler to inline the method in the generated code.\nI think the compiler is smart enough to find that out by itself\nanyhow though)\n\nThanks\n j\n\n> >\n> > > +\n> > > +\tconst std::string &devnode() const;\n> > > +\tconst struct media_device_info &info() const;\n> > > +\tstd::vector entities() const;\n> > > +\n> > > +\tbool lookup(const std::string &name, std::string &devnode) const;\n> > > +\n> > > +private:\n> > > +\tbool acquired_;\n> > > +\n> > > +\tstd::string devnode_;\n> > > +\tstruct media_device_info info_;\n> > > +\tstd::map entities_;\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > +\n> > > +#endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index 52b556a8ed4050cb..17cdf06dd2bedfb3 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -1,10 +1,12 @@\n> > > libcamera_sources = files([\n> > > 'camera.cpp',\n> > > + 'deviceenumerator.cpp',\n> > > 'log.cpp',\n> > > 'main.cpp',\n> > > ])\n> > >\n> > > libcamera_headers = files([\n> > > + 'include/deviceenumerator.h',\n> > > 'include/log.h',\n> > > 'include/utils.h',\n> > > ])\n> > > --\n> > > 2.20.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n>\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AB8FF60B2F\n\tfor ;\n\tFri, 28 Dec 2018 09:21:48 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id EF4CF100003;\n\tFri, 28 Dec 2018 08:21:47 +0000 (UTC)"],"Date":"Fri, 28 Dec 2018 09:21:48 +0100","From":"Jacopo Mondi ","To":"Niklas S??derlund ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181228082148.GC909@uno.localdomain>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-4-niklas.soderlund@ragnatech.se>\n\t<20181224104216.GC2364@archlinux>\n\t<20181228030657.GP19796@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"7DO5AaGCk89r4vaK\"","Content-Disposition":"inline","In-Reply-To":"<20181228030657.GP19796@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.11.1 (2018-12-01)","Subject":"Re: [libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator:\n\tadd DeviceInfo 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":"Fri, 28 Dec 2018 08:21:48 -0000"}},{"id":103,"web_url":"https://patchwork.libcamera.org/comment/103/","msgid":"<1655792.Gx1Dx5KDSo@avalon>","date":"2018-12-28T17:15:20","subject":"Re: [libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator:\n\tadd DeviceInfo class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Friday, 28 December 2018 10:21:48 EET Jacopo Mondi wrote:\n> On Fri, Dec 28, 2018 at 04:06:57AM +0100, Niklas S??derlund wrote:\n> > On 2018-12-24 11:42:16 +0100, Jacopo Mondi wrote:\n> > > On Sun, Dec 23, 2018 at 12:00:32AM +0100, Niklas S??derlund wrote:\n> > > > Provide a DeviceInfo class which holds all information from the\n> > > > initial enumeration of a media device. Not all information available\n> > > > at a media device is stored, only the information needed for a\n> > > > pipeline handler to find a specific device.\n> > > > \n> > > > Signed-off-by: Niklas Söderlund \n> > > > ---\n> > > > \n> > > > src/libcamera/deviceenumerator.cpp | 78 +++++++++++++++++++++++\n> > > > src/libcamera/include/deviceenumerator.h | 44 +++++++++++++\n> > > > src/libcamera/meson.build | 2 +\n> > > > 3 files changed, 124 insertions(+)\n> > > > create mode 100644 src/libcamera/deviceenumerator.cpp\n> > > > create mode 100644 src/libcamera/include/deviceenumerator.h\n> > > > \n> > > > diff --git a/src/libcamera/deviceenumerator.cpp\n> > > > b/src/libcamera/deviceenumerator.cpp new file mode 100644\n> > > > index 0000000000000000..7c44a65b45472ef3\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/deviceenumerator.cpp\n> > > > @@ -0,0 +1,78 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2018, Google Inc.\n> > > > + *\n> > > > + * deviceenumerator.cpp - Enumeration and matching\n> > > > + */\n> > > > +\n> > > > +#include \"deviceenumerator.h\"\n> > > > +#include \"log.h\"\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +/*\n> > > > ---------------------------------------------------------------------\n> > > > -------- + * DeviceInfo\n> > > > + */\n> > > > +\n> > > > +DeviceInfo::DeviceInfo(const std::string &devnode, const struct\n> > > > media_device_info &info, +\t\t const std::map > > > std::string> &entities)\n> > > > +\t: acquired_(false), devnode_(devnode), info_(info),\n> > > > entities_(entities)\n> > > > +{\n> > > > +\tfor (const auto &entity : entities_)\n> > > > +\t\tLOG(Info) << \"Device: \" << devnode_ << \" Entity: '\" << \nentity.first\n> > > > << \"' -> \" << entity.second; +}\n> > > > +\n> > > > +int DeviceInfo::acquire()\n> > > > +{\n> > > > +\tif (acquired_)\n> > > > +\t\treturn -EBUSY;\n> > > > +\n> > > > +\tacquired_ = true;\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > +void DeviceInfo::release()\n> > > > +{\n> > > > +\tacquired_ = false;\n> > > > +}\n> > > > +\n> > > > +bool DeviceInfo::busy() const\n> > > > +{\n> > > > +\treturn acquired_;\n> > > > +}\n> > > > +\n> > > > +const std::string &DeviceInfo::devnode() const\n> > > > +{\n> > > > +\treturn devnode_;\n> > > > +}\n> > > > +\n> > > > +const struct media_device_info &DeviceInfo::info() const\n> > > > +{\n> > > > +\treturn info_;\n> > > > +}\n> > > > +\n> > > > +std::vector DeviceInfo::entities() const\n> > > > +{\n> > > > +\tstd::vector entities;\n> > > > +\n> > > > +\tfor (const auto &entity : entities_)\n> > > > +\t\tentities.push_back(entity.first);\n> > > > +\n> > > > +\treturn entities;\n> > > \n> > > Are you returning (by copy) a variable with local scope?\n> > > Why coldn't you just return (as const reference maybe) entities_ ?\n> > \n> > This is by design. The idea is that a new copy of the entities in a\n> > vector instead of the class internal object of the model of entities as\n> > the use-case here is to do matching. Why would it be beneficial to\n> > return a internal data structure?\n> \n> It would avoid moving around a vector by copy. You only use\n> the returned vector to compare entities names, so you could just\n> simply return a const reference to your internal data structure.\n> \n> I would actually ask why would it be beneficial to copy all entities\n> names in a new vector and copy it back, if that's not going to be\n> modified anyhow. Furthermore, you could have used operator= to copy\n> the internal one:\n> http://www.cplusplus.com/reference/vector/vector/operator=/\n\nWhat do you mean by \"copying it back\" ? Have you overlooked the fact that \nentities_ is a map, not a vector ?\n\n> > > > +}\n> > > > +\n> > > > +bool DeviceInfo::lookup(const std::string &name, std::string\n> > > > &devnode) const +{\n> > > > +\tauto it = entities_.find(name);\n> > > > +\n> > > > +\tif (it == entities_.end()) {\n> > > > +\t\tLOG(Error) << \"Trying to lookup entity '\" << name << \"' which do\n> > > > not exist\";\n> > > > \n> > > nit: \"does not exist\"\n> > \n> > My comprehension of the Queens English is lacking so I trust you on\n> \n> Don't!\n> \n> > this. For my education is not 'does' used when there is more then one\n> > while 'do' is used when there is exactly one?\n> \n> You might be right, let's wait for some more educated (or native)\n> speaker to school us.\n\nI'm certainly not native, and wouldn't claim to be more educated, but I think \nthe subject of the verb refers to \"entity\", which is a third person singular, \nso \"does\" is the right form.\n\n> > > > +\t\treturn false;\n> > > > +\t}\n> > > > +\n> > > > +\tdevnode = it->second;\n> > > > +\treturn true;\n> > > > +}\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/libcamera/include/deviceenumerator.h\n> > > > b/src/libcamera/include/deviceenumerator.h new file mode 100644\n> > > > index 0000000000000000..0136ed6ea23bf65e\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/include/deviceenumerator.h\n> > > > @@ -0,0 +1,44 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2018, Google Inc.\n> > > > + *\n> > > > + * deviceenumerator.h - API to enumerate and find media devices\n> > > > + */\n> > > > +#ifndef __LIBCAMERA_DEVICEENUMERATE_H__\n> > > > +#define __LIBCAMERA_DEVICEENUMERATE_H__\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +class DeviceInfo\n> > > > +{\n> > > > +public:\n> > > > +\tDeviceInfo(const std::string &devnode, const struct\n> > > > media_device_info &info, +\t\t const std::map > > > std::string> &entities);\n> > > > +\n> > > > +\tint acquire();\n> > > > +\tvoid release();\n> > > > +\tbool busy() const;\n> > > \n> > > Trivial methods might be inlined here.\n> > \n> > There are other methods who can't be inlined for this class. Why spread\n> > the implementation around?\n> \n> I'm in favour of inlining single line and trivial methods/constructors as\n> much as we could. It reduce the size of the cpp file, and makes clear\n> from reading the header the method is trivial. I'm not much concerned about\n> spreading per se, as we all navigate tags don't we?\n\nMethods should only be inlined when the code size is roughly equivalent to a \nfunction call. Trivial getters are good candidates. Constructors may be, but \nonly if they don't have a large list of member initializers. For destructors, \neven if the function is empty, it might call other destructors for members, \nand the destructor of the base class. Inlining destructors is thus usually a \nbad idea. Inlinline in C++ uses a different syntax than in C, but the result \nis similar, the code is inlined, and may thus increase the binary size. Please \nsee the Google C++ style guide section about this.\n\n> (let alone the discussion about the fact inlining in class declaration\n> is a hint for the compiler to inline the method in the generated code.\n> I think the compiler is smart enough to find that out by itself\n> anyhow though)\n> \n> > > > +\n> > > > +\tconst std::string &devnode() const;\n> > > > +\tconst struct media_device_info &info() const;\n> > > > +\tstd::vector entities() const;\n> > > > +\n> > > > +\tbool lookup(const std::string &name, std::string &devnode) const;\n> > > > +\n> > > > +private:\n> > > > +\tbool acquired_;\n> > > > +\n> > > > +\tstd::string devnode_;\n> > > > +\tstruct media_device_info info_;\n> > > > +\tstd::map entities_;\n> > > > +};\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > +\n> > > > +#endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */\n\n[snip]","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 D9F4E60B2E\n\tfor ;\n\tFri, 28 Dec 2018 18:14:22 +0100 (CET)","from avalon.localnet (unknown\n\t[IPv6:2a02:2788:66a:3eb:2624:a446:f4b7:b19d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5CDB9DF;\n\tFri, 28 Dec 2018 18:14:22 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546017262;\n\tbh=epIMTLBRjuoPBCbbMPcSoa//pFMNGLOgWfrrpADtixI=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=j1o5sBu+0tO1w6S03Myhw/K0PzIUOBOSfFJY4vFF2bVaxv8RNXZ4sM3stWDjg0h6z\n\tyveEb2xDYEc8x/QbHYy1ZFJf6iG4GbSZoCNZqBppooblYFrUnVqadNBdqZwu3emYEP\n\tmZ95tnqAg7xLZ3tjbvJnUZxwvwyKhMIvh9BsxIUk=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Fri, 28 Dec 2018 19:15:20 +0200","Message-ID":"<1655792.Gx1Dx5KDSo@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181228082148.GC909@uno.localdomain>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181228030657.GP19796@bigcity.dyn.berto.se>\n\t<20181228082148.GC909@uno.localdomain>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator:\n\tadd DeviceInfo 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":"Fri, 28 Dec 2018 17:14:25 -0000"}},{"id":104,"web_url":"https://patchwork.libcamera.org/comment/104/","msgid":"<4182491.7EdO3GXL8K@avalon>","date":"2018-12-28T17:20:59","subject":"Re: [libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator:\n\tadd DeviceInfo class","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 Sunday, 23 December 2018 01:00:32 EET Niklas Söderlund wrote:\n> Provide a DeviceInfo class which holds all information from the initial\n> enumeration of a media device. Not all information available at a media\n> device is stored, only the information needed for a pipeline handler to\n> find a specific device.\n\nDo I understand correctly that this will be replaced with MediaDevice ? If so \nI only have a small comment below, and for the rest,\n\nReviewed-by: Laurent Pinchart \n\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/deviceenumerator.cpp | 78 ++++++++++++++++++++++++\n> src/libcamera/include/deviceenumerator.h | 44 +++++++++++++\n> src/libcamera/meson.build | 2 +\n> 3 files changed, 124 insertions(+)\n> create mode 100644 src/libcamera/deviceenumerator.cpp\n> create mode 100644 src/libcamera/include/deviceenumerator.h\n> \n> diff --git a/src/libcamera/deviceenumerator.cpp\n> b/src/libcamera/deviceenumerator.cpp new file mode 100644\n> index 0000000000000000..7c44a65b45472ef3\n> --- /dev/null\n> +++ b/src/libcamera/deviceenumerator.cpp\n> @@ -0,0 +1,78 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * deviceenumerator.cpp - Enumeration and matching\n> + */\n> +\n> +#include \"deviceenumerator.h\"\n> +#include \"log.h\"\n> +\n> +namespace libcamera {\n> +\n> +/* ------------------------------------------------------------------------\n> + * DeviceInfo\n> + */\n> +\n> +DeviceInfo::DeviceInfo(const std::string &devnode, const struct\n> media_device_info &info,\n> +\t\t const std::map\n> &entities)\n> +\t: acquired_(false), devnode_(devnode), info_(info), entities_(entities)\n> +{\n> +\tfor (const auto &entity : entities_)\n> +\t\tLOG(Info) << \"Device: \" << devnode_ << \" Entity: '\" << entity.first << \n\"'\n> -> \" << entity.second;\n> +}\n> +\n> +int DeviceInfo::acquire()\n> +{\n> +\tif (acquired_)\n> +\t\treturn -EBUSY;\n> +\n> +\tacquired_ = true;\n> +\n> +\treturn 0;\n> +}\n> +\n> +void DeviceInfo::release()\n> +{\n> +\tacquired_ = false;\n> +}\n> +\n> +bool DeviceInfo::busy() const\n> +{\n> +\treturn acquired_;\n> +}\n> +\n> +const std::string &DeviceInfo::devnode() const\n> +{\n> +\treturn devnode_;\n> +}\n> +\n> +const struct media_device_info &DeviceInfo::info() const\n> +{\n> +\treturn info_;\n> +}\n> +\n> +std::vector DeviceInfo::entities() const\n> +{\n> +\tstd::vector entities;\n> +\n> +\tfor (const auto &entity : entities_)\n> +\t\tentities.push_back(entity.first);\n> +\n> +\treturn entities;\n> +}\n> +\n> +bool DeviceInfo::lookup(const std::string &name, std::string &devnode)\n> const\n> +{\n> +\tauto it = entities_.find(name);\n> +\n> +\tif (it == entities_.end()) {\n> +\t\tLOG(Error) << \"Trying to lookup entity '\" << name << \"' which do not\n> exist\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\tdevnode = it->second;\n> +\treturn true;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/include/deviceenumerator.h\n> b/src/libcamera/include/deviceenumerator.h new file mode 100644\n> index 0000000000000000..0136ed6ea23bf65e\n> --- /dev/null\n> +++ b/src/libcamera/include/deviceenumerator.h\n> @@ -0,0 +1,44 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * deviceenumerator.h - API to enumerate and find media devices\n> + */\n> +#ifndef __LIBCAMERA_DEVICEENUMERATE_H__\n\nThis doesn't match the file name.\n\n> +#define __LIBCAMERA_DEVICEENUMERATE_H__\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class DeviceInfo\n> +{\n> +public:\n> +\tDeviceInfo(const std::string &devnode, const struct media_device_info\n> &info,\n> +\t\t const std::map &entities);\n> +\n> +\tint acquire();\n> +\tvoid release();\n> +\tbool busy() const;\n> +\n> +\tconst std::string &devnode() const;\n> +\tconst struct media_device_info &info() const;\n> +\tstd::vector entities() const;\n> +\n> +\tbool lookup(const std::string &name, std::string &devnode) const;\n> +\n> +private:\n> +\tbool acquired_;\n> +\n> +\tstd::string devnode_;\n> +\tstruct media_device_info info_;\n> +\tstd::map entities_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */\n\n[snip]","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 8415660B2E\n\tfor ;\n\tFri, 28 Dec 2018 18:20:02 +0100 (CET)","from avalon.localnet (unknown\n\t[IPv6:2a02:2788:66a:3eb:2624:a446:f4b7:b19d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E6FF4DF;\n\tFri, 28 Dec 2018 18:20:01 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546017602;\n\tbh=Of1ejUJOzPAX5b/i1TrkccDquj1hNomjGDwGeh7gObQ=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=skBQfjYoJBAOGtH+4fRI3D9YYtQFvecEu3BSx4BGRzCP5dTEFb3n/is705C9EyEOq\n\tW8xGTtzogG2M0fwxfrtZALiR+QWgmDJqJMI+VL98OQrig8EqapIih8tG6og2ae7qiw\n\tAv7SdpfGWJhIiHCaWrpBOwlV99V4Ro+enliAuON4=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Fri, 28 Dec 2018 19:20:59 +0200","Message-ID":"<4182491.7EdO3GXL8K@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181222230041.29999-4-niklas.soderlund@ragnatech.se>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-4-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator:\n\tadd DeviceInfo 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":"Fri, 28 Dec 2018 17:20:02 -0000"}},{"id":112,"web_url":"https://patchwork.libcamera.org/comment/112/","msgid":"<20181229004600.GZ19796@bigcity.dyn.berto.se>","date":"2018-12-29T00:46:01","subject":"Re: [libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator:\n\tadd DeviceInfo class","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 comments.\n\nOn 2018-12-28 19:20:59 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Sunday, 23 December 2018 01:00:32 EET Niklas Söderlund wrote:\n> > Provide a DeviceInfo class which holds all information from the initial\n> > enumeration of a media device. Not all information available at a media\n> > device is stored, only the information needed for a pipeline handler to\n> > find a specific device.\n> \n> Do I understand correctly that this will be replaced with MediaDevice ? If so \n> I only have a small comment below, and for the rest,\n\nYes the plan is to replace this with MediaDevice.\n\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/deviceenumerator.cpp | 78 ++++++++++++++++++++++++\n> > src/libcamera/include/deviceenumerator.h | 44 +++++++++++++\n> > src/libcamera/meson.build | 2 +\n> > 3 files changed, 124 insertions(+)\n> > create mode 100644 src/libcamera/deviceenumerator.cpp\n> > create mode 100644 src/libcamera/include/deviceenumerator.h\n> > \n> > diff --git a/src/libcamera/deviceenumerator.cpp\n> > b/src/libcamera/deviceenumerator.cpp new file mode 100644\n> > index 0000000000000000..7c44a65b45472ef3\n> > --- /dev/null\n> > +++ b/src/libcamera/deviceenumerator.cpp\n> > @@ -0,0 +1,78 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * deviceenumerator.cpp - Enumeration and matching\n> > + */\n> > +\n> > +#include \"deviceenumerator.h\"\n> > +#include \"log.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +/* ------------------------------------------------------------------------\n> > + * DeviceInfo\n> > + */\n> > +\n> > +DeviceInfo::DeviceInfo(const std::string &devnode, const struct\n> > media_device_info &info,\n> > +\t\t const std::map\n> > &entities)\n> > +\t: acquired_(false), devnode_(devnode), info_(info), entities_(entities)\n> > +{\n> > +\tfor (const auto &entity : entities_)\n> > +\t\tLOG(Info) << \"Device: \" << devnode_ << \" Entity: '\" << entity.first << \n> \"'\n> > -> \" << entity.second;\n> > +}\n> > +\n> > +int DeviceInfo::acquire()\n> > +{\n> > +\tif (acquired_)\n> > +\t\treturn -EBUSY;\n> > +\n> > +\tacquired_ = true;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void DeviceInfo::release()\n> > +{\n> > +\tacquired_ = false;\n> > +}\n> > +\n> > +bool DeviceInfo::busy() const\n> > +{\n> > +\treturn acquired_;\n> > +}\n> > +\n> > +const std::string &DeviceInfo::devnode() const\n> > +{\n> > +\treturn devnode_;\n> > +}\n> > +\n> > +const struct media_device_info &DeviceInfo::info() const\n> > +{\n> > +\treturn info_;\n> > +}\n> > +\n> > +std::vector DeviceInfo::entities() const\n> > +{\n> > +\tstd::vector entities;\n> > +\n> > +\tfor (const auto &entity : entities_)\n> > +\t\tentities.push_back(entity.first);\n> > +\n> > +\treturn entities;\n> > +}\n> > +\n> > +bool DeviceInfo::lookup(const std::string &name, std::string &devnode)\n> > const\n> > +{\n> > +\tauto it = entities_.find(name);\n> > +\n> > +\tif (it == entities_.end()) {\n> > +\t\tLOG(Error) << \"Trying to lookup entity '\" << name << \"' which do not\n> > exist\";\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tdevnode = it->second;\n> > +\treturn true;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/include/deviceenumerator.h\n> > b/src/libcamera/include/deviceenumerator.h new file mode 100644\n> > index 0000000000000000..0136ed6ea23bf65e\n> > --- /dev/null\n> > +++ b/src/libcamera/include/deviceenumerator.h\n> > @@ -0,0 +1,44 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * deviceenumerator.h - API to enumerate and find media devices\n> > + */\n> > +#ifndef __LIBCAMERA_DEVICEENUMERATE_H__\n> \n> This doesn't match the file name.\n\nThanks for noticing, fixed.\n\n> \n> > +#define __LIBCAMERA_DEVICEENUMERATE_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +class DeviceInfo\n> > +{\n> > +public:\n> > +\tDeviceInfo(const std::string &devnode, const struct media_device_info\n> > &info,\n> > +\t\t const std::map &entities);\n> > +\n> > +\tint acquire();\n> > +\tvoid release();\n> > +\tbool busy() const;\n> > +\n> > +\tconst std::string &devnode() const;\n> > +\tconst struct media_device_info &info() const;\n> > +\tstd::vector entities() const;\n> > +\n> > +\tbool lookup(const std::string &name, std::string &devnode) const;\n> > +\n> > +private:\n> > +\tbool acquired_;\n> > +\n> > +\tstd::string devnode_;\n> > +\tstruct media_device_info info_;\n> > +\tstd::map entities_;\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */\n> \n> [snip]\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> \n>","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 C0B2460B30\n\tfor ;\n\tSat, 29 Dec 2018 01:46:02 +0100 (CET)","by mail-lf1-x142.google.com with SMTP id n18so15457382lfh.6\n\tfor ;\n\tFri, 28 Dec 2018 16:46:02 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tj197sm8250010lfe.24.2018.12.28.16.46.01\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 28 Dec 2018 16:46:01 -0800 (PST)"],"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=VJPXesT4gcpESoUcOoT6fFC+phuIPQh1iG0Jo3X/GGc=;\n\tb=MgB2i1FSVVnALBcMNSQZAB4/seQypjMaax/+sTQl/qzs8qGWwWjmsevVvqgwjR9a1J\n\tHdgHEqAF44Elp5Nkdo8XAL9hR8DAORXzsTeaXzgO774rzgrSteIBOdmbyHwhchy3hhXA\n\tZtibT9Cg199mc6DGZS9/iJwzE/vALV6RO7PR68A+LxuGLn8e2Q7utoBT0k7GN3ThUglc\n\tuvErE1vWmaLYhzerXnOlQ9eu6s5cFevg1dLn2Kt1+zBBjI1fZ0/MYG28aACEILQMKZCw\n\tmhCLQNghM1wsYLL6Rmjk27f2hp6e7baFle4IED0tENJWZSfdkSKzwh02ww35iHNKJMqW\n\tasNQ==","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=VJPXesT4gcpESoUcOoT6fFC+phuIPQh1iG0Jo3X/GGc=;\n\tb=lED5xA5MoX1q8LVQQzCHo/kweUv/dPF+6ZXbzGHJkfc5Qg9CIqpgYm9/RyFBKC7d7j\n\toqEc3KjKqEeSnqp0uZzxXJvs65j/bp1WZ77xW/3PmhIRPWwyo5QVCyiEqVhBXE8NndSU\n\tCtJRwUDi0pD8So1UmMytgC2+ZRStXYjU8X7e2BPPF4ifc3puX4cPyUNREhhz+o7a0ha2\n\tm3DGgmQfZMeUTPL6uMnPQrZkcwt7/OKsOVplB6T4n8d1R6wZad3XCoPO/oIAu8vj+hRe\n\t61Hj0l6QS+9jBKx/HHI3LBFQDcLEzXvzVNUusobbktGACgmIIeI8ndcDJM3Jn1ccDx1d\n\tHO1g==","X-Gm-Message-State":"AA+aEWY2diOjvMv6KFVbYYi3vhvWCK4GGNgyJHfs0wCWa1Abh2yJ6Asv\n\tp/aTy6j9kwJmKPKJgxd2GQzTkg==","X-Google-Smtp-Source":"AFSGD/Vassgf2jPuM1cqJCbFUOfBg8Bzb9zrNoKW1Nesd7o3yHfzXjrTzB2xecnt3cmpAzm8zmbF4g==","X-Received":"by 2002:a19:c650:: with SMTP id\n\tw77mr15289854lff.56.1546044362091; \n\tFri, 28 Dec 2018 16:46:02 -0800 (PST)","Date":"Sat, 29 Dec 2018 01:46:01 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181229004600.GZ19796@bigcity.dyn.berto.se>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-4-niklas.soderlund@ragnatech.se>\n\t<4182491.7EdO3GXL8K@avalon>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<4182491.7EdO3GXL8K@avalon>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator:\n\tadd DeviceInfo 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, 29 Dec 2018 00:46:03 -0000"}},{"id":121,"web_url":"https://patchwork.libcamera.org/comment/121/","msgid":"<20181229092935.sabgpqduaspp2tlu@uno.localdomain>","date":"2018-12-29T09:29:35","subject":"Re: [libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator:\n\tadd DeviceInfo class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Dec 28, 2018 at 07:15:20PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Friday, 28 December 2018 10:21:48 EET Jacopo Mondi wrote:\n> > On Fri, Dec 28, 2018 at 04:06:57AM +0100, Niklas S??derlund wrote:\n> > > On 2018-12-24 11:42:16 +0100, Jacopo Mondi wrote:\n> > > > On Sun, Dec 23, 2018 at 12:00:32AM +0100, Niklas S??derlund wrote:\n> > > > > Provide a DeviceInfo class which holds all information from the\n> > > > > initial enumeration of a media device. Not all information available\n> > > > > at a media device is stored, only the information needed for a\n> > > > > pipeline handler to find a specific device.\n> > > > >\n> > > > > Signed-off-by: Niklas Söderlund \n> > > > > ---\n> > > > >\n> > > > > src/libcamera/deviceenumerator.cpp | 78 +++++++++++++++++++++++\n> > > > > src/libcamera/include/deviceenumerator.h | 44 +++++++++++++\n> > > > > src/libcamera/meson.build | 2 +\n> > > > > 3 files changed, 124 insertions(+)\n> > > > > create mode 100644 src/libcamera/deviceenumerator.cpp\n> > > > > create mode 100644 src/libcamera/include/deviceenumerator.h\n> > > > >\n> > > > > diff --git a/src/libcamera/deviceenumerator.cpp\n> > > > > b/src/libcamera/deviceenumerator.cpp new file mode 100644\n> > > > > index 0000000000000000..7c44a65b45472ef3\n> > > > > --- /dev/null\n> > > > > +++ b/src/libcamera/deviceenumerator.cpp\n> > > > > @@ -0,0 +1,78 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2018, Google Inc.\n> > > > > + *\n> > > > > + * deviceenumerator.cpp - Enumeration and matching\n> > > > > + */\n> > > > > +\n> > > > > +#include \"deviceenumerator.h\"\n> > > > > +#include \"log.h\"\n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +/*\n> > > > > ---------------------------------------------------------------------\n> > > > > -------- + * DeviceInfo\n> > > > > + */\n> > > > > +\n> > > > > +DeviceInfo::DeviceInfo(const std::string &devnode, const struct\n> > > > > media_device_info &info, +\t\t const std::map > > > > std::string> &entities)\n> > > > > +\t: acquired_(false), devnode_(devnode), info_(info),\n> > > > > entities_(entities)\n> > > > > +{\n> > > > > +\tfor (const auto &entity : entities_)\n> > > > > +\t\tLOG(Info) << \"Device: \" << devnode_ << \" Entity: '\" <<\n> entity.first\n> > > > > << \"' -> \" << entity.second; +}\n> > > > > +\n> > > > > +int DeviceInfo::acquire()\n> > > > > +{\n> > > > > +\tif (acquired_)\n> > > > > +\t\treturn -EBUSY;\n> > > > > +\n> > > > > +\tacquired_ = true;\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > +}\n> > > > > +\n> > > > > +void DeviceInfo::release()\n> > > > > +{\n> > > > > +\tacquired_ = false;\n> > > > > +}\n> > > > > +\n> > > > > +bool DeviceInfo::busy() const\n> > > > > +{\n> > > > > +\treturn acquired_;\n> > > > > +}\n> > > > > +\n> > > > > +const std::string &DeviceInfo::devnode() const\n> > > > > +{\n> > > > > +\treturn devnode_;\n> > > > > +}\n> > > > > +\n> > > > > +const struct media_device_info &DeviceInfo::info() const\n> > > > > +{\n> > > > > +\treturn info_;\n> > > > > +}\n> > > > > +\n> > > > > +std::vector DeviceInfo::entities() const\n> > > > > +{\n> > > > > +\tstd::vector entities;\n> > > > > +\n> > > > > +\tfor (const auto &entity : entities_)\n> > > > > +\t\tentities.push_back(entity.first);\n> > > > > +\n> > > > > +\treturn entities;\n> > > >\n> > > > Are you returning (by copy) a variable with local scope?\n> > > > Why coldn't you just return (as const reference maybe) entities_ ?\n> > >\n> > > This is by design. The idea is that a new copy of the entities in a\n> > > vector instead of the class internal object of the model of entities as\n> > > the use-case here is to do matching. Why would it be beneficial to\n> > > return a internal data structure?\n> >\n> > It would avoid moving around a vector by copy. You only use\n> > the returned vector to compare entities names, so you could just\n> > simply return a const reference to your internal data structure.\n> >\n> > I would actually ask why would it be beneficial to copy all entities\n> > names in a new vector and copy it back, if that's not going to be\n> > modified anyhow. Furthermore, you could have used operator= to copy\n> > the internal one:\n> > http://www.cplusplus.com/reference/vector/vector/operator=/\n>\n> What do you mean by \"copying it back\" ? Have you overlooked the fact that\n> entities_ is a map, not a vector ?\n>\n\nIndeed I did.\nSorry for the noise.\n\nThanks\n j\n\n> > > > > +}\n> > > > > +\n> > > > > +bool DeviceInfo::lookup(const std::string &name, std::string\n> > > > > &devnode) const +{\n> > > > > +\tauto it = entities_.find(name);\n> > > > > +\n> > > > > +\tif (it == entities_.end()) {\n> > > > > +\t\tLOG(Error) << \"Trying to lookup entity '\" << name << \"' which do\n> > > > > not exist\";\n> > > > >\n> > > > nit: \"does not exist\"\n> > >\n> > > My comprehension of the Queens English is lacking so I trust you on\n> >\n> > Don't!\n> >\n> > > this. For my education is not 'does' used when there is more then one\n> > > while 'do' is used when there is exactly one?\n> >\n> > You might be right, let's wait for some more educated (or native)\n> > speaker to school us.\n>\n> I'm certainly not native, and wouldn't claim to be more educated, but I think\n> the subject of the verb refers to \"entity\", which is a third person singular,\n> so \"does\" is the right form.\n>\n> > > > > +\t\treturn false;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tdevnode = it->second;\n> > > > > +\treturn true;\n> > > > > +}\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > > > > diff --git a/src/libcamera/include/deviceenumerator.h\n> > > > > b/src/libcamera/include/deviceenumerator.h new file mode 100644\n> > > > > index 0000000000000000..0136ed6ea23bf65e\n> > > > > --- /dev/null\n> > > > > +++ b/src/libcamera/include/deviceenumerator.h\n> > > > > @@ -0,0 +1,44 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2018, Google Inc.\n> > > > > + *\n> > > > > + * deviceenumerator.h - API to enumerate and find media devices\n> > > > > + */\n> > > > > +#ifndef __LIBCAMERA_DEVICEENUMERATE_H__\n> > > > > +#define __LIBCAMERA_DEVICEENUMERATE_H__\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +class DeviceInfo\n> > > > > +{\n> > > > > +public:\n> > > > > +\tDeviceInfo(const std::string &devnode, const struct\n> > > > > media_device_info &info, +\t\t const std::map > > > > std::string> &entities);\n> > > > > +\n> > > > > +\tint acquire();\n> > > > > +\tvoid release();\n> > > > > +\tbool busy() const;\n> > > >\n> > > > Trivial methods might be inlined here.\n> > >\n> > > There are other methods who can't be inlined for this class. Why spread\n> > > the implementation around?\n> >\n> > I'm in favour of inlining single line and trivial methods/constructors as\n> > much as we could. It reduce the size of the cpp file, and makes clear\n> > from reading the header the method is trivial. I'm not much concerned about\n> > spreading per se, as we all navigate tags don't we?\n>\n> Methods should only be inlined when the code size is roughly equivalent to a\n> function call. Trivial getters are good candidates. Constructors may be, but\n> only if they don't have a large list of member initializers. For destructors,\n> even if the function is empty, it might call other destructors for members,\n> and the destructor of the base class. Inlining destructors is thus usually a\n> bad idea. Inlinline in C++ uses a different syntax than in C, but the result\n> is similar, the code is inlined, and may thus increase the binary size. Please\n> see the Google C++ style guide section about this.\n>\n> > (let alone the discussion about the fact inlining in class declaration\n> > is a hint for the compiler to inline the method in the generated code.\n> > I think the compiler is smart enough to find that out by itself\n> > anyhow though)\n> >\n> > > > > +\n> > > > > +\tconst std::string &devnode() const;\n> > > > > +\tconst struct media_device_info &info() const;\n> > > > > +\tstd::vector entities() const;\n> > > > > +\n> > > > > +\tbool lookup(const std::string &name, std::string &devnode) const;\n> > > > > +\n> > > > > +private:\n> > > > > +\tbool acquired_;\n> > > > > +\n> > > > > +\tstd::string devnode_;\n> > > > > +\tstruct media_device_info info_;\n> > > > > +\tstd::map entities_;\n> > > > > +};\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > > > > +\n> > > > > +#endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */\n>\n> [snip]\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n>\n>","headers":{"Return-Path":"","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 215EC600CC\n\tfor ;\n\tSat, 29 Dec 2018 10:29:39 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 5965DFF804;\n\tSat, 29 Dec 2018 09:29:38 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Sat, 29 Dec 2018 10:29:35 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tNiklas S??derlund ","Message-ID":"<20181229092935.sabgpqduaspp2tlu@uno.localdomain>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181228030657.GP19796@bigcity.dyn.berto.se>\n\t<20181228082148.GC909@uno.localdomain> <1655792.Gx1Dx5KDSo@avalon>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"z33rebuqdcnrtksq\"","Content-Disposition":"inline","In-Reply-To":"<1655792.Gx1Dx5KDSo@avalon>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 03/12] libcamera: deviceenumerator:\n\tadd DeviceInfo 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, 29 Dec 2018 09:29:39 -0000"}}]