[{"id":3536,"web_url":"https://patchwork.libcamera.org/comment/3536/","msgid":"<20200120132732.c2efqu2zb5a4wguu@uno.localdomain>","date":"2020-01-20T13:27:32","subject":"Re: [libcamera-devel] [PATCH 05/19] libcamera: Replace ARRAY_SIZE\n\twith std::array","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jan 20, 2020 at 02:24:23AM +0200, Laurent Pinchart wrote:\n> Replace the C-style arrays with std::array wherever the ARRAY_SIZE macro\n> is used, removing the need for the macro completely. std::array combines\n> the performance and accessibility of C-style arrays with the benefits of\n> a standard container, which is shown here through the ability to carry\n> its size.\n>\n\nyou know my opinion on std::array already :)\n\nAnyway\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/camera.cpp | 10 +++++-----\n> src/libcamera/include/utils.h | 2 --\n> src/libcamera/ipa_module.cpp | 8 ++++----\n> src/libcamera/log.cpp | 15 ++++++++-------\n> src/libcamera/utils.cpp | 5 -----\n> src/libcamera/v4l2_videodevice.cpp | 7 ++++---\n> test/ipc/unixsocket.cpp | 8 ++++----\n> 7 files changed, 25 insertions(+), 30 deletions(-)\n>\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 79a5f994f9bb..3385c08778b8 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -7,6 +7,7 @@\n>\n> #include \n>\n> +#include \n> #include \n>\n> #include \n> @@ -15,7 +16,6 @@\n>\n> #include \"log.h\"\n> #include \"pipeline_handler.h\"\n> -#include \"utils.h\"\n>\n> /**\n> * \\file camera.h\n> @@ -404,7 +404,7 @@ Camera::~Camera()\n> \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> }\n>\n> -static const char *const camera_state_names[] = {\n> +static constexpr std::array camera_state_names = {\n> \t\"Available\",\n> \t\"Acquired\",\n> \t\"Configured\",\n> @@ -416,8 +416,8 @@ bool Camera::stateBetween(State low, State high) const\n> \tif (state_ >= low && state_ <= high)\n> \t\treturn true;\n>\n> -\tASSERT(static_cast(low) < ARRAY_SIZE(camera_state_names) &&\n> -\t static_cast(high) < ARRAY_SIZE(camera_state_names));\n> +\tASSERT(static_cast(low) < camera_state_names.size() &&\n> +\t static_cast(high) < camera_state_names.size());\n>\n> \tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> \t\t\t << \" state trying operation requiring state between \"\n> @@ -432,7 +432,7 @@ bool Camera::stateIs(State state) const\n> \tif (state_ == state)\n> \t\treturn true;\n>\n> -\tASSERT(static_cast(state) < ARRAY_SIZE(camera_state_names));\n> +\tASSERT(static_cast(state) < camera_state_names.size());\n>\n> \tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> \t\t\t << \" state trying operation requiring state \"\n> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h\n> index e467eb21c518..f55c52f72bd5 100644\n> --- a/src/libcamera/include/utils.h\n> +++ b/src/libcamera/include/utils.h\n> @@ -15,8 +15,6 @@\n> #include \n> #include \n>\n> -#define ARRAY_SIZE(a)\t(sizeof(a) / sizeof(a[0]))\n> -\n> #ifndef __DOXYGEN__\n>\n> /* uClibc and uClibc-ng don't provide O_TMPFILE */\n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index 2c355ea8b5e5..9c927a62b308 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -22,7 +22,6 @@\n>\n> #include \"log.h\"\n> #include \"pipeline_handler.h\"\n> -#include \"utils.h\"\n\nYou should include... Ah no, it's there for no reason already :)\n\nAll the rest is good\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n>\n> /**\n> * \\file ipa_module.h\n> @@ -480,7 +479,7 @@ bool IPAModule::match(PipelineHandler *pipe,\n> */\n> bool IPAModule::isOpenSource() const\n> {\n> -\tstatic const char *osLicenses[] = {\n> +\tstatic constexpr std::array osLicenses = {\n> \t\t\"GPL-2.0-only\",\n> \t\t\"GPL-2.0-or-later\",\n> \t\t\"GPL-3.0-only\",\n> @@ -491,9 +490,10 @@ bool IPAModule::isOpenSource() const\n> \t\t\"LGPL-3.0-or-later\",\n> \t};\n>\n> -\tfor (unsigned int i = 0; i < ARRAY_SIZE(osLicenses); i++)\n> -\t\tif (!strcmp(osLicenses[i], info_.license))\n> +\tfor (const char *license : osLicenses) {\n> +\t\tif (!strcmp(license, info_.license))\n> \t\t\treturn true;\n> +\t}\n>\n> \treturn false;\n> }\n> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp\n> index 1dac4666b435..ef0b81f77131 100644\n> --- a/src/libcamera/log.cpp\n> +++ b/src/libcamera/log.cpp\n> @@ -7,6 +7,7 @@\n>\n> #include \"log.h\"\n>\n> +#include \n> #if HAVE_BACKTRACE\n> #include \n> #endif\n> @@ -83,7 +84,7 @@ static int log_severity_to_syslog(LogSeverity severity)\n>\n> static const char *log_severity_name(LogSeverity severity)\n> {\n> -\tstatic const char *const names[] = {\n> +\tstatic constexpr std::array names = {\n> \t\t\" DBG\",\n> \t\t\" INFO\",\n> \t\t\" WARN\",\n> @@ -91,7 +92,7 @@ static const char *log_severity_name(LogSeverity severity)\n> \t\t\"FATAL\",\n> \t};\n>\n> -\tif (static_cast(severity) < ARRAY_SIZE(names))\n> +\tif (static_cast(severity) < names.size())\n> \t\treturn names[severity];\n> \telse\n> \t\treturn \"UNKWN\";\n> @@ -405,9 +406,9 @@ void Logger::backtrace()\n> \tif (!output)\n> \t\treturn;\n>\n> -\tvoid *buffer[32];\n> -\tint num_entries = ::backtrace(buffer, ARRAY_SIZE(buffer));\n> -\tchar **strings = backtrace_symbols(buffer, num_entries);\n> +\tstd::array buffer;\n> +\tint num_entries = ::backtrace(buffer.data(), buffer.size());\n> +\tchar **strings = backtrace_symbols(buffer.data(), num_entries);\n> \tif (!strings)\n> \t\treturn;\n>\n> @@ -603,7 +604,7 @@ void Logger::parseLogLevels()\n> */\n> LogSeverity Logger::parseLogLevel(const std::string &level)\n> {\n> -\tstatic const char *const names[] = {\n> +\tstatic constexpr std::array names = {\n> \t\t\"DEBUG\",\n> \t\t\"INFO\",\n> \t\t\"WARN\",\n> @@ -620,7 +621,7 @@ LogSeverity Logger::parseLogLevel(const std::string &level)\n> \t\t\tseverity = LogInvalid;\n> \t} else {\n> \t\tseverity = LogInvalid;\n> -\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(names); ++i) {\n> +\t\tfor (unsigned int i = 0; i < names.size(); ++i) {\n> \t\t\tif (names[i] == level) {\n> \t\t\t\tseverity = i;\n> \t\t\t\tbreak;\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> index 4beffdab5eb6..74576797ee77 100644\n> --- a/src/libcamera/utils.cpp\n> +++ b/src/libcamera/utils.cpp\n> @@ -22,11 +22,6 @@ namespace libcamera {\n>\n> namespace utils {\n>\n> -/**\n> - * \\def ARRAY_SIZE(array)\n> - * \\brief Determine the number of elements in the static array.\n> - */\n> -\n> /**\n> * \\brief Strip the directory prefix from the path\n> * \\param[in] path The path to process\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 18220b81af21..51be1dcd7fff 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -7,6 +7,7 @@\n>\n> #include \"v4l2_videodevice.h\"\n>\n> +#include \n> #include \n> #include \n> #include \n> @@ -991,13 +992,13 @@ int V4L2VideoDevice::exportBuffers(unsigned int count,\n>\n> \tfor (unsigned i = 0; i < count; ++i) {\n> \t\tstruct v4l2_buffer buf = {};\n> -\t\tstruct v4l2_plane planes[VIDEO_MAX_PLANES] = {};\n> +\t\tstd::array planes = {};\n>\n> \t\tbuf.index = i;\n> \t\tbuf.type = bufferType_;\n> \t\tbuf.memory = memoryType_;\n> -\t\tbuf.length = ARRAY_SIZE(planes);\n> -\t\tbuf.m.planes = planes;\n> +\t\tbuf.length = planes.size();\n> +\t\tbuf.m.planes = planes.data();\n>\n> \t\tret = ioctl(VIDIOC_QUERYBUF, &buf);\n> \t\tif (ret < 0) {\n> diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp\n> index f53042b88720..5bf543c197fa 100644\n> --- a/test/ipc/unixsocket.cpp\n> +++ b/test/ipc/unixsocket.cpp\n> @@ -6,6 +6,7 @@\n> */\n>\n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -21,7 +22,6 @@\n> #include \"ipc_unixsocket.h\"\n> #include \"test.h\"\n> #include \"thread.h\"\n> -#include \"utils.h\"\n>\n> #define CMD_CLOSE\t0\n> #define CMD_REVERSE\t1\n> @@ -303,13 +303,13 @@ protected:\n> \t\tIPCUnixSocket::Payload message, response;\n> \t\tint ret;\n>\n> -\t\tstatic const char *strings[2] = {\n> +\t\tstatic constexpr std::array strings = {\n> \t\t\t\"Foo\",\n> \t\t\t\"Bar\",\n> \t\t};\n> \t\tint fds[2];\n>\n> -\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(strings); i++) {\n> +\t\tfor (unsigned int i = 0; i < strings.size(); i++) {\n> \t\t\tunsigned int len = strlen(strings[i]);\n>\n> \t\t\tfds[i] = open(\"/tmp\", O_TMPFILE | O_RDWR,\n> @@ -331,7 +331,7 @@ protected:\n> \t\tif (ret)\n> \t\t\treturn ret;\n>\n> -\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(strings); i++) {\n> +\t\tfor (unsigned int i = 0; i < strings.size(); i++) {\n> \t\t\tunsigned int len = strlen(strings[i]);\n> \t\t\tchar buf[len];\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D48A60804\n\tfor ;\n\tMon, 20 Jan 2020 14:25:01 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 1783C100007;\n\tMon, 20 Jan 2020 13:25:00 +0000 (UTC)"],"Date":"Mon, 20 Jan 2020 14:27:32 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200120132732.c2efqu2zb5a4wguu@uno.localdomain>","References":"<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>\n\t<20200120002437.6633-6-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"ckmglhagijmybwwy\"","Content-Disposition":"inline","In-Reply-To":"<20200120002437.6633-6-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 05/19] libcamera: Replace ARRAY_SIZE\n\twith std::array","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 20 Jan 2020 13:25:01 -0000"}},{"id":3538,"web_url":"https://patchwork.libcamera.org/comment/3538/","msgid":"","date":"2020-01-20T13:36:52","subject":"Re: [libcamera-devel] [PATCH 05/19] libcamera: Replace ARRAY_SIZE\n\twith std::array","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 20/01/2020 00:24, Laurent Pinchart wrote:\n> Replace the C-style arrays with std::array wherever the ARRAY_SIZE macro\n> is used, removing the need for the macro completely. std::array combines\n> the performance and accessibility of C-style arrays with the benefits of\n> a standard container, which is shown here through the ability to carry\n> its size.\n> \n\n\"... at the expense of no longer being able to determine the array size\nat compile time.\"\n\nSometimes it's nice to be able to just add to an array and know that the\naddition will be picked up correctly. (Adding to static lists etc).\n\nHo hum though ... this is probably progress and forces the developer to\nthink more about the allocations appropriately.\n\nReviewed-by: Kieran Bingham \n\n\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/camera.cpp | 10 +++++-----\n> src/libcamera/include/utils.h | 2 --\n> src/libcamera/ipa_module.cpp | 8 ++++----\n> src/libcamera/log.cpp | 15 ++++++++-------\n> src/libcamera/utils.cpp | 5 -----\n> src/libcamera/v4l2_videodevice.cpp | 7 ++++---\n> test/ipc/unixsocket.cpp | 8 ++++----\n> 7 files changed, 25 insertions(+), 30 deletions(-)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 79a5f994f9bb..3385c08778b8 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -7,6 +7,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> \n> #include \n> @@ -15,7 +16,6 @@\n> \n> #include \"log.h\"\n> #include \"pipeline_handler.h\"\n> -#include \"utils.h\"\n> \n> /**\n> * \\file camera.h\n> @@ -404,7 +404,7 @@ Camera::~Camera()\n> \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> }\n> \n> -static const char *const camera_state_names[] = {\n> +static constexpr std::array camera_state_names = {\n> \t\"Available\",\n> \t\"Acquired\",\n> \t\"Configured\",\n> @@ -416,8 +416,8 @@ bool Camera::stateBetween(State low, State high) const\n> \tif (state_ >= low && state_ <= high)\n> \t\treturn true;\n> \n> -\tASSERT(static_cast(low) < ARRAY_SIZE(camera_state_names) &&\n> -\t static_cast(high) < ARRAY_SIZE(camera_state_names));\n> +\tASSERT(static_cast(low) < camera_state_names.size() &&\n> +\t static_cast(high) < camera_state_names.size());\n> \n> \tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> \t\t\t << \" state trying operation requiring state between \"\n> @@ -432,7 +432,7 @@ bool Camera::stateIs(State state) const\n> \tif (state_ == state)\n> \t\treturn true;\n> \n> -\tASSERT(static_cast(state) < ARRAY_SIZE(camera_state_names));\n> +\tASSERT(static_cast(state) < camera_state_names.size());\n> \n> \tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> \t\t\t << \" state trying operation requiring state \"\n> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h\n> index e467eb21c518..f55c52f72bd5 100644\n> --- a/src/libcamera/include/utils.h\n> +++ b/src/libcamera/include/utils.h\n> @@ -15,8 +15,6 @@\n> #include \n> #include \n> \n> -#define ARRAY_SIZE(a)\t(sizeof(a) / sizeof(a[0]))\n> -\n> #ifndef __DOXYGEN__\n> \n> /* uClibc and uClibc-ng don't provide O_TMPFILE */\n> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> index 2c355ea8b5e5..9c927a62b308 100644\n> --- a/src/libcamera/ipa_module.cpp\n> +++ b/src/libcamera/ipa_module.cpp\n> @@ -22,7 +22,6 @@\n> \n> #include \"log.h\"\n> #include \"pipeline_handler.h\"\n> -#include \"utils.h\"\n> \n> /**\n> * \\file ipa_module.h\n> @@ -480,7 +479,7 @@ bool IPAModule::match(PipelineHandler *pipe,\n> */\n> bool IPAModule::isOpenSource() const\n> {\n> -\tstatic const char *osLicenses[] = {\n> +\tstatic constexpr std::array osLicenses = {\n\nOh, so we can't do compile time determination of the size of arrays,\nWe must codify the size in advance?\n\nThat feels a bit like we're losing a feature (or convenience) ...\n\nWill the compiler warn(or error) if the given size does not match the\nnumber of elements given?\n\nI guess it might happily over-allocate and prevent under-allocation?\n\n\n\n> \t\t\"GPL-2.0-only\",\n> \t\t\"GPL-2.0-or-later\",\n> \t\t\"GPL-3.0-only\",\n> @@ -491,9 +490,10 @@ bool IPAModule::isOpenSource() const\n> \t\t\"LGPL-3.0-or-later\",\n> \t};\n> \n> -\tfor (unsigned int i = 0; i < ARRAY_SIZE(osLicenses); i++)\n> -\t\tif (!strcmp(osLicenses[i], info_.license))\n> +\tfor (const char *license : osLicenses) {\n> +\t\tif (!strcmp(license, info_.license))\n> \t\t\treturn true;\n> +\t}\n> \n> \treturn false;\n> }\n> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp\n> index 1dac4666b435..ef0b81f77131 100644\n> --- a/src/libcamera/log.cpp\n> +++ b/src/libcamera/log.cpp\n> @@ -7,6 +7,7 @@\n> \n> #include \"log.h\"\n> \n> +#include \n> #if HAVE_BACKTRACE\n> #include \n> #endif\n> @@ -83,7 +84,7 @@ static int log_severity_to_syslog(LogSeverity severity)\n> \n> static const char *log_severity_name(LogSeverity severity)\n> {\n> -\tstatic const char *const names[] = {\n> +\tstatic constexpr std::array names = {\n\nHaving to explicitly set the sizes seems like we're doing the job of the\ncompiler...\n\n> \t\t\" DBG\",\n> \t\t\" INFO\",\n> \t\t\" WARN\",\n> @@ -91,7 +92,7 @@ static const char *log_severity_name(LogSeverity severity)\n> \t\t\"FATAL\",\n> \t};\n> \n> -\tif (static_cast(severity) < ARRAY_SIZE(names))\n> +\tif (static_cast(severity) < names.size())\n\nBut this reads nicely otherwise...\n\n\n> \t\treturn names[severity];\n> \telse\n> \t\treturn \"UNKWN\";\n> @@ -405,9 +406,9 @@ void Logger::backtrace()\n> \tif (!output)\n> \t\treturn;\n> \n> -\tvoid *buffer[32];\n> -\tint num_entries = ::backtrace(buffer, ARRAY_SIZE(buffer));\n> -\tchar **strings = backtrace_symbols(buffer, num_entries);\n> +\tstd::array buffer;\n> +\tint num_entries = ::backtrace(buffer.data(), buffer.size());\n> +\tchar **strings = backtrace_symbols(buffer.data(), num_entries);\n> \tif (!strings)\n> \t\treturn;\n> \n> @@ -603,7 +604,7 @@ void Logger::parseLogLevels()\n> */\n> LogSeverity Logger::parseLogLevel(const std::string &level)\n> {\n> -\tstatic const char *const names[] = {\n> +\tstatic constexpr std::array names = {\n> \t\t\"DEBUG\",\n> \t\t\"INFO\",\n> \t\t\"WARN\",\n> @@ -620,7 +621,7 @@ LogSeverity Logger::parseLogLevel(const std::string &level)\n> \t\t\tseverity = LogInvalid;\n> \t} else {\n> \t\tseverity = LogInvalid;\n> -\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(names); ++i) {\n> +\t\tfor (unsigned int i = 0; i < names.size(); ++i) {\n> \t\t\tif (names[i] == level) {\n> \t\t\t\tseverity = i;\n> \t\t\t\tbreak;\n> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> index 4beffdab5eb6..74576797ee77 100644\n> --- a/src/libcamera/utils.cpp\n> +++ b/src/libcamera/utils.cpp\n> @@ -22,11 +22,6 @@ namespace libcamera {\n> \n> namespace utils {\n> \n> -/**\n> - * \\def ARRAY_SIZE(array)\n> - * \\brief Determine the number of elements in the static array.\n> - */\n> -\n> /**\n> * \\brief Strip the directory prefix from the path\n> * \\param[in] path The path to process\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 18220b81af21..51be1dcd7fff 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -7,6 +7,7 @@\n> \n> #include \"v4l2_videodevice.h\"\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -991,13 +992,13 @@ int V4L2VideoDevice::exportBuffers(unsigned int count,\n> \n> \tfor (unsigned i = 0; i < count; ++i) {\n> \t\tstruct v4l2_buffer buf = {};\n> -\t\tstruct v4l2_plane planes[VIDEO_MAX_PLANES] = {};\n> +\t\tstd::array planes = {};\n> \n> \t\tbuf.index = i;\n> \t\tbuf.type = bufferType_;\n> \t\tbuf.memory = memoryType_;\n> -\t\tbuf.length = ARRAY_SIZE(planes);\n> -\t\tbuf.m.planes = planes;\n> +\t\tbuf.length = planes.size();\n> +\t\tbuf.m.planes = planes.data();\n> \n> \t\tret = ioctl(VIDIOC_QUERYBUF, &buf);\n> \t\tif (ret < 0) {\n> diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp\n> index f53042b88720..5bf543c197fa 100644\n> --- a/test/ipc/unixsocket.cpp\n> +++ b/test/ipc/unixsocket.cpp\n> @@ -6,6 +6,7 @@\n> */\n> \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -21,7 +22,6 @@\n> #include \"ipc_unixsocket.h\"\n> #include \"test.h\"\n> #include \"thread.h\"\n> -#include \"utils.h\"\n> \n> #define CMD_CLOSE\t0\n> #define CMD_REVERSE\t1\n> @@ -303,13 +303,13 @@ protected:\n> \t\tIPCUnixSocket::Payload message, response;\n> \t\tint ret;\n> \n> -\t\tstatic const char *strings[2] = {\n> +\t\tstatic constexpr std::array strings = {\n> \t\t\t\"Foo\",\n> \t\t\t\"Bar\",\n> \t\t};\n> \t\tint fds[2];\n> \n> -\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(strings); i++) {\n> +\t\tfor (unsigned int i = 0; i < strings.size(); i++) {\n> \t\t\tunsigned int len = strlen(strings[i]);\n> \n> \t\t\tfds[i] = open(\"/tmp\", O_TMPFILE | O_RDWR,\n> @@ -331,7 +331,7 @@ protected:\n> \t\tif (ret)\n> \t\t\treturn ret;\n> \n> -\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(strings); i++) {\n> +\t\tfor (unsigned int i = 0; i < strings.size(); i++) {\n> \t\t\tunsigned int len = strlen(strings[i]);\n> \t\t\tchar buf[len];\n> \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 4EDE760804\n\tfor ;\n\tMon, 20 Jan 2020 14:36:56 +0100 (CET)","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 B375EA62;\n\tMon, 20 Jan 2020 14:36:55 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579527415;\n\tbh=q6aqTtdFsz8Yh/ymHo6UCSkxhPAohWbvQ3dUUs0FUlc=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=DWJ5vRqh+vNfJ26JknbmwMP7bJDkcBa5eoRdqx12yH1MATLvkWyqqadctMNV11P0S\n\t5GLx/Gya9LXjVovN7qDPRF7oC+VFGBxLqbBLQmYB3CjUXOBNx3XsCLegsbDBmHhvmk\n\tZ26C01ag+yDddW66aZYqxHh7g457Nrdqvf9skb6I=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>\n\t<20200120002437.6633-6-laurent.pinchart@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\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Mon, 20 Jan 2020 13:36:52 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.0","MIME-Version":"1.0","In-Reply-To":"<20200120002437.6633-6-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 05/19] libcamera: Replace ARRAY_SIZE\n\twith std::array","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 20 Jan 2020 13:36:56 -0000"}},{"id":3547,"web_url":"https://patchwork.libcamera.org/comment/3547/","msgid":"<20200120162745.GH20122@pendragon.ideasonboard.com>","date":"2020-01-20T16:27:45","subject":"Re: [libcamera-devel] [PATCH 05/19] libcamera: Replace ARRAY_SIZE\n\twith std::array","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, Jan 20, 2020 at 01:36:52PM +0000, Kieran Bingham wrote:\n> On 20/01/2020 00:24, Laurent Pinchart wrote:\n> > Replace the C-style arrays with std::array wherever the ARRAY_SIZE macro\n> > is used, removing the need for the macro completely. std::array combines\n> > the performance and accessibility of C-style arrays with the benefits of\n> > a standard container, which is shown here through the ability to carry\n> > its size.\n> \n> \"... at the expense of no longer being able to determine the array size\n> at compile time.\"\n> \n> Sometimes it's nice to be able to just add to an array and know that the\n> addition will be picked up correctly. (Adding to static lists etc).\n> \n> Ho hum though ... this is probably progress and forces the developer to\n> think more about the allocations appropriately.\n> \n> Reviewed-by: Kieran Bingham \n> \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/camera.cpp | 10 +++++-----\n> > src/libcamera/include/utils.h | 2 --\n> > src/libcamera/ipa_module.cpp | 8 ++++----\n> > src/libcamera/log.cpp | 15 ++++++++-------\n> > src/libcamera/utils.cpp | 5 -----\n> > src/libcamera/v4l2_videodevice.cpp | 7 ++++---\n> > test/ipc/unixsocket.cpp | 8 ++++----\n> > 7 files changed, 25 insertions(+), 30 deletions(-)\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 79a5f994f9bb..3385c08778b8 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -7,6 +7,7 @@\n> > \n> > #include \n> > \n> > +#include \n> > #include \n> > \n> > #include \n> > @@ -15,7 +16,6 @@\n> > \n> > #include \"log.h\"\n> > #include \"pipeline_handler.h\"\n> > -#include \"utils.h\"\n> > \n> > /**\n> > * \\file camera.h\n> > @@ -404,7 +404,7 @@ Camera::~Camera()\n> > \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> > }\n> > \n> > -static const char *const camera_state_names[] = {\n> > +static constexpr std::array camera_state_names = {\n> > \t\"Available\",\n> > \t\"Acquired\",\n> > \t\"Configured\",\n> > @@ -416,8 +416,8 @@ bool Camera::stateBetween(State low, State high) const\n> > \tif (state_ >= low && state_ <= high)\n> > \t\treturn true;\n> > \n> > -\tASSERT(static_cast(low) < ARRAY_SIZE(camera_state_names) &&\n> > -\t static_cast(high) < ARRAY_SIZE(camera_state_names));\n> > +\tASSERT(static_cast(low) < camera_state_names.size() &&\n> > +\t static_cast(high) < camera_state_names.size());\n> > \n> > \tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> > \t\t\t << \" state trying operation requiring state between \"\n> > @@ -432,7 +432,7 @@ bool Camera::stateIs(State state) const\n> > \tif (state_ == state)\n> > \t\treturn true;\n> > \n> > -\tASSERT(static_cast(state) < ARRAY_SIZE(camera_state_names));\n> > +\tASSERT(static_cast(state) < camera_state_names.size());\n> > \n> > \tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> > \t\t\t << \" state trying operation requiring state \"\n> > diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h\n> > index e467eb21c518..f55c52f72bd5 100644\n> > --- a/src/libcamera/include/utils.h\n> > +++ b/src/libcamera/include/utils.h\n> > @@ -15,8 +15,6 @@\n> > #include \n> > #include \n> > \n> > -#define ARRAY_SIZE(a)\t(sizeof(a) / sizeof(a[0]))\n> > -\n> > #ifndef __DOXYGEN__\n> > \n> > /* uClibc and uClibc-ng don't provide O_TMPFILE */\n> > diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> > index 2c355ea8b5e5..9c927a62b308 100644\n> > --- a/src/libcamera/ipa_module.cpp\n> > +++ b/src/libcamera/ipa_module.cpp\n> > @@ -22,7 +22,6 @@\n> > \n> > #include \"log.h\"\n> > #include \"pipeline_handler.h\"\n> > -#include \"utils.h\"\n> > \n> > /**\n> > * \\file ipa_module.h\n> > @@ -480,7 +479,7 @@ bool IPAModule::match(PipelineHandler *pipe,\n> > */\n> > bool IPAModule::isOpenSource() const\n> > {\n> > -\tstatic const char *osLicenses[] = {\n> > +\tstatic constexpr std::array osLicenses = {\n> \n> Oh, so we can't do compile time determination of the size of arrays,\n> We must codify the size in advance?\n> \n> That feels a bit like we're losing a feature (or convenience) ...\n\nThat's the part that bothers me too :-( It's fixed in\nhttps://en.cppreference.com/w/cpp/experimental/make_array but that's an\nexperimental feature, available in stdlibc++ from g++-6 onwards but not\nin libc++. We could however pull it to our utils namespace.\n\n> Will the compiler warn(or error) if the given size does not match the\n> number of elements given?\n> \n> I guess it might happily over-allocate and prevent under-allocation?\n\nYes, and that can actually cause bugs, it's a blocker. I'll send a v2\nwith an implementation of std::make_array() in utils to fix this.\n\n> > \t\t\"GPL-2.0-only\",\n> > \t\t\"GPL-2.0-or-later\",\n> > \t\t\"GPL-3.0-only\",\n> > @@ -491,9 +490,10 @@ bool IPAModule::isOpenSource() const\n> > \t\t\"LGPL-3.0-or-later\",\n> > \t};\n> > \n> > -\tfor (unsigned int i = 0; i < ARRAY_SIZE(osLicenses); i++)\n> > -\t\tif (!strcmp(osLicenses[i], info_.license))\n> > +\tfor (const char *license : osLicenses) {\n> > +\t\tif (!strcmp(license, info_.license))\n> > \t\t\treturn true;\n> > +\t}\n> > \n> > \treturn false;\n> > }\n> > diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp\n> > index 1dac4666b435..ef0b81f77131 100644\n> > --- a/src/libcamera/log.cpp\n> > +++ b/src/libcamera/log.cpp\n> > @@ -7,6 +7,7 @@\n> > \n> > #include \"log.h\"\n> > \n> > +#include \n> > #if HAVE_BACKTRACE\n> > #include \n> > #endif\n> > @@ -83,7 +84,7 @@ static int log_severity_to_syslog(LogSeverity severity)\n> > \n> > static const char *log_severity_name(LogSeverity severity)\n> > {\n> > -\tstatic const char *const names[] = {\n> > +\tstatic constexpr std::array names = {\n> \n> Having to explicitly set the sizes seems like we're doing the job of the\n> compiler...\n\nYes, utils::make_array() should fix this.\n\n> > \t\t\" DBG\",\n> > \t\t\" INFO\",\n> > \t\t\" WARN\",\n> > @@ -91,7 +92,7 @@ static const char *log_severity_name(LogSeverity severity)\n> > \t\t\"FATAL\",\n> > \t};\n> > \n> > -\tif (static_cast(severity) < ARRAY_SIZE(names))\n> > +\tif (static_cast(severity) < names.size())\n> \n> But this reads nicely otherwise...\n\nAnd I also like the \"for const char *license : osLicenses)\" above.\n\n> > \t\treturn names[severity];\n> > \telse\n> > \t\treturn \"UNKWN\";\n> > @@ -405,9 +406,9 @@ void Logger::backtrace()\n> > \tif (!output)\n> > \t\treturn;\n> > \n> > -\tvoid *buffer[32];\n> > -\tint num_entries = ::backtrace(buffer, ARRAY_SIZE(buffer));\n> > -\tchar **strings = backtrace_symbols(buffer, num_entries);\n> > +\tstd::array buffer;\n> > +\tint num_entries = ::backtrace(buffer.data(), buffer.size());\n> > +\tchar **strings = backtrace_symbols(buffer.data(), num_entries);\n> > \tif (!strings)\n> > \t\treturn;\n> > \n> > @@ -603,7 +604,7 @@ void Logger::parseLogLevels()\n> > */\n> > LogSeverity Logger::parseLogLevel(const std::string &level)\n> > {\n> > -\tstatic const char *const names[] = {\n> > +\tstatic constexpr std::array names = {\n> > \t\t\"DEBUG\",\n> > \t\t\"INFO\",\n> > \t\t\"WARN\",\n> > @@ -620,7 +621,7 @@ LogSeverity Logger::parseLogLevel(const std::string &level)\n> > \t\t\tseverity = LogInvalid;\n> > \t} else {\n> > \t\tseverity = LogInvalid;\n> > -\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(names); ++i) {\n> > +\t\tfor (unsigned int i = 0; i < names.size(); ++i) {\n> > \t\t\tif (names[i] == level) {\n> > \t\t\t\tseverity = i;\n> > \t\t\t\tbreak;\n> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > index 4beffdab5eb6..74576797ee77 100644\n> > --- a/src/libcamera/utils.cpp\n> > +++ b/src/libcamera/utils.cpp\n> > @@ -22,11 +22,6 @@ namespace libcamera {\n> > \n> > namespace utils {\n> > \n> > -/**\n> > - * \\def ARRAY_SIZE(array)\n> > - * \\brief Determine the number of elements in the static array.\n> > - */\n> > -\n> > /**\n> > * \\brief Strip the directory prefix from the path\n> > * \\param[in] path The path to process\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 18220b81af21..51be1dcd7fff 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -7,6 +7,7 @@\n> > \n> > #include \"v4l2_videodevice.h\"\n> > \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -991,13 +992,13 @@ int V4L2VideoDevice::exportBuffers(unsigned int count,\n> > \n> > \tfor (unsigned i = 0; i < count; ++i) {\n> > \t\tstruct v4l2_buffer buf = {};\n> > -\t\tstruct v4l2_plane planes[VIDEO_MAX_PLANES] = {};\n> > +\t\tstd::array planes = {};\n> > \n> > \t\tbuf.index = i;\n> > \t\tbuf.type = bufferType_;\n> > \t\tbuf.memory = memoryType_;\n> > -\t\tbuf.length = ARRAY_SIZE(planes);\n> > -\t\tbuf.m.planes = planes;\n> > +\t\tbuf.length = planes.size();\n> > +\t\tbuf.m.planes = planes.data();\n> > \n> > \t\tret = ioctl(VIDIOC_QUERYBUF, &buf);\n> > \t\tif (ret < 0) {\n> > diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp\n> > index f53042b88720..5bf543c197fa 100644\n> > --- a/test/ipc/unixsocket.cpp\n> > +++ b/test/ipc/unixsocket.cpp\n> > @@ -6,6 +6,7 @@\n> > */\n> > \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -21,7 +22,6 @@\n> > #include \"ipc_unixsocket.h\"\n> > #include \"test.h\"\n> > #include \"thread.h\"\n> > -#include \"utils.h\"\n> > \n> > #define CMD_CLOSE\t0\n> > #define CMD_REVERSE\t1\n> > @@ -303,13 +303,13 @@ protected:\n> > \t\tIPCUnixSocket::Payload message, response;\n> > \t\tint ret;\n> > \n> > -\t\tstatic const char *strings[2] = {\n> > +\t\tstatic constexpr std::array strings = {\n> > \t\t\t\"Foo\",\n> > \t\t\t\"Bar\",\n> > \t\t};\n> > \t\tint fds[2];\n> > \n> > -\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(strings); i++) {\n> > +\t\tfor (unsigned int i = 0; i < strings.size(); i++) {\n> > \t\t\tunsigned int len = strlen(strings[i]);\n> > \n> > \t\t\tfds[i] = open(\"/tmp\", O_TMPFILE | O_RDWR,\n> > @@ -331,7 +331,7 @@ protected:\n> > \t\tif (ret)\n> > \t\t\treturn ret;\n> > \n> > -\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(strings); i++) {\n> > +\t\tfor (unsigned int i = 0; i < strings.size(); i++) {\n> > \t\t\tunsigned int len = strlen(strings[i]);\n> > \t\t\tchar buf[len];\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 092DF60804\n\tfor ;\n\tMon, 20 Jan 2020 17:27:47 +0100 (CET)","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 64896A62;\n\tMon, 20 Jan 2020 17:27:46 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579537666;\n\tbh=VnxpROCr7zpbNc06ukAMZ+p1/2+aZU8KAu1/760Afmc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NGukThY2RUCUkhPSKcRAKMbXw+94N8uSLsZSbpc/U//VFAqOVwaVa5W1C6wKV/8H2\n\tuCdLh+npMZkCzP4olLBmGk5K1u7SwhHaByCH9VQ0zW2qyrXmu54++P/u507PBN/CnL\n\t/js/VrRDDThscwfFAe6jd7e2EpGI9CpQ6HJQPTNg=","Date":"Mon, 20 Jan 2020 18:27:45 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200120162745.GH20122@pendragon.ideasonboard.com>","References":"<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>\n\t<20200120002437.6633-6-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 05/19] libcamera: Replace ARRAY_SIZE\n\twith std::array","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 20 Jan 2020 16:27:47 -0000"}},{"id":3548,"web_url":"https://patchwork.libcamera.org/comment/3548/","msgid":"<20200120163447.GI20122@pendragon.ideasonboard.com>","date":"2020-01-20T16:34:47","subject":"Re: [libcamera-devel] [PATCH 05/19] libcamera: Replace ARRAY_SIZE\n\twith std::array","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jan 20, 2020 at 02:27:32PM +0100, Jacopo Mondi wrote:\n> On Mon, Jan 20, 2020 at 02:24:23AM +0200, Laurent Pinchart wrote:\n> > Replace the C-style arrays with std::array wherever the ARRAY_SIZE macro\n> > is used, removing the need for the macro completely. std::array combines\n> > the performance and accessibility of C-style arrays with the benefits of\n> > a standard container, which is shown here through the ability to carry\n> > its size.\n> \n> you know my opinion on std::array already :)\n\nI think you're not alone with that opinion :-) I'm personally not fully\nin love with std::array in all cases, but there are clear upsides. I\ndon't intend in pushing this patch if there's no consensus. I'll send a\nv2 that will introduce utils::make_array() to fix one of the issues,\nlet's see what to do from there.\n\n> Anyway\n> Reviewed-by: Jacopo Mondi \n> \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/camera.cpp | 10 +++++-----\n> > src/libcamera/include/utils.h | 2 --\n> > src/libcamera/ipa_module.cpp | 8 ++++----\n> > src/libcamera/log.cpp | 15 ++++++++-------\n> > src/libcamera/utils.cpp | 5 -----\n> > src/libcamera/v4l2_videodevice.cpp | 7 ++++---\n> > test/ipc/unixsocket.cpp | 8 ++++----\n> > 7 files changed, 25 insertions(+), 30 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 79a5f994f9bb..3385c08778b8 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -7,6 +7,7 @@\n> >\n> > #include \n> >\n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -15,7 +16,6 @@\n> >\n> > #include \"log.h\"\n> > #include \"pipeline_handler.h\"\n> > -#include \"utils.h\"\n> >\n> > /**\n> > * \\file camera.h\n> > @@ -404,7 +404,7 @@ Camera::~Camera()\n> > \t\tLOG(Camera, Error) << \"Removing camera while still in use\";\n> > }\n> >\n> > -static const char *const camera_state_names[] = {\n> > +static constexpr std::array camera_state_names = {\n> > \t\"Available\",\n> > \t\"Acquired\",\n> > \t\"Configured\",\n> > @@ -416,8 +416,8 @@ bool Camera::stateBetween(State low, State high) const\n> > \tif (state_ >= low && state_ <= high)\n> > \t\treturn true;\n> >\n> > -\tASSERT(static_cast(low) < ARRAY_SIZE(camera_state_names) &&\n> > -\t static_cast(high) < ARRAY_SIZE(camera_state_names));\n> > +\tASSERT(static_cast(low) < camera_state_names.size() &&\n> > +\t static_cast(high) < camera_state_names.size());\n> >\n> > \tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> > \t\t\t << \" state trying operation requiring state between \"\n> > @@ -432,7 +432,7 @@ bool Camera::stateIs(State state) const\n> > \tif (state_ == state)\n> > \t\treturn true;\n> >\n> > -\tASSERT(static_cast(state) < ARRAY_SIZE(camera_state_names));\n> > +\tASSERT(static_cast(state) < camera_state_names.size());\n> >\n> > \tLOG(Camera, Debug) << \"Camera in \" << camera_state_names[state_]\n> > \t\t\t << \" state trying operation requiring state \"\n> > diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h\n> > index e467eb21c518..f55c52f72bd5 100644\n> > --- a/src/libcamera/include/utils.h\n> > +++ b/src/libcamera/include/utils.h\n> > @@ -15,8 +15,6 @@\n> > #include \n> > #include \n> >\n> > -#define ARRAY_SIZE(a)\t(sizeof(a) / sizeof(a[0]))\n> > -\n> > #ifndef __DOXYGEN__\n> >\n> > /* uClibc and uClibc-ng don't provide O_TMPFILE */\n> > diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp\n> > index 2c355ea8b5e5..9c927a62b308 100644\n> > --- a/src/libcamera/ipa_module.cpp\n> > +++ b/src/libcamera/ipa_module.cpp\n> > @@ -22,7 +22,6 @@\n> >\n> > #include \"log.h\"\n> > #include \"pipeline_handler.h\"\n> > -#include \"utils.h\"\n> \n> You should include... Ah no, it's there for no reason already :)\n> \n> All the rest is good\n> Reviewed-by: Jacopo Mondi \n> \n> Thanks\n> j\n> >\n> > /**\n> > * \\file ipa_module.h\n> > @@ -480,7 +479,7 @@ bool IPAModule::match(PipelineHandler *pipe,\n> > */\n> > bool IPAModule::isOpenSource() const\n> > {\n> > -\tstatic const char *osLicenses[] = {\n> > +\tstatic constexpr std::array osLicenses = {\n> > \t\t\"GPL-2.0-only\",\n> > \t\t\"GPL-2.0-or-later\",\n> > \t\t\"GPL-3.0-only\",\n> > @@ -491,9 +490,10 @@ bool IPAModule::isOpenSource() const\n> > \t\t\"LGPL-3.0-or-later\",\n> > \t};\n> >\n> > -\tfor (unsigned int i = 0; i < ARRAY_SIZE(osLicenses); i++)\n> > -\t\tif (!strcmp(osLicenses[i], info_.license))\n> > +\tfor (const char *license : osLicenses) {\n> > +\t\tif (!strcmp(license, info_.license))\n> > \t\t\treturn true;\n> > +\t}\n> >\n> > \treturn false;\n> > }\n> > diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp\n> > index 1dac4666b435..ef0b81f77131 100644\n> > --- a/src/libcamera/log.cpp\n> > +++ b/src/libcamera/log.cpp\n> > @@ -7,6 +7,7 @@\n> >\n> > #include \"log.h\"\n> >\n> > +#include \n> > #if HAVE_BACKTRACE\n> > #include \n> > #endif\n> > @@ -83,7 +84,7 @@ static int log_severity_to_syslog(LogSeverity severity)\n> >\n> > static const char *log_severity_name(LogSeverity severity)\n> > {\n> > -\tstatic const char *const names[] = {\n> > +\tstatic constexpr std::array names = {\n> > \t\t\" DBG\",\n> > \t\t\" INFO\",\n> > \t\t\" WARN\",\n> > @@ -91,7 +92,7 @@ static const char *log_severity_name(LogSeverity severity)\n> > \t\t\"FATAL\",\n> > \t};\n> >\n> > -\tif (static_cast(severity) < ARRAY_SIZE(names))\n> > +\tif (static_cast(severity) < names.size())\n> > \t\treturn names[severity];\n> > \telse\n> > \t\treturn \"UNKWN\";\n> > @@ -405,9 +406,9 @@ void Logger::backtrace()\n> > \tif (!output)\n> > \t\treturn;\n> >\n> > -\tvoid *buffer[32];\n> > -\tint num_entries = ::backtrace(buffer, ARRAY_SIZE(buffer));\n> > -\tchar **strings = backtrace_symbols(buffer, num_entries);\n> > +\tstd::array buffer;\n> > +\tint num_entries = ::backtrace(buffer.data(), buffer.size());\n> > +\tchar **strings = backtrace_symbols(buffer.data(), num_entries);\n> > \tif (!strings)\n> > \t\treturn;\n> >\n> > @@ -603,7 +604,7 @@ void Logger::parseLogLevels()\n> > */\n> > LogSeverity Logger::parseLogLevel(const std::string &level)\n> > {\n> > -\tstatic const char *const names[] = {\n> > +\tstatic constexpr std::array names = {\n> > \t\t\"DEBUG\",\n> > \t\t\"INFO\",\n> > \t\t\"WARN\",\n> > @@ -620,7 +621,7 @@ LogSeverity Logger::parseLogLevel(const std::string &level)\n> > \t\t\tseverity = LogInvalid;\n> > \t} else {\n> > \t\tseverity = LogInvalid;\n> > -\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(names); ++i) {\n> > +\t\tfor (unsigned int i = 0; i < names.size(); ++i) {\n> > \t\t\tif (names[i] == level) {\n> > \t\t\t\tseverity = i;\n> > \t\t\t\tbreak;\n> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp\n> > index 4beffdab5eb6..74576797ee77 100644\n> > --- a/src/libcamera/utils.cpp\n> > +++ b/src/libcamera/utils.cpp\n> > @@ -22,11 +22,6 @@ namespace libcamera {\n> >\n> > namespace utils {\n> >\n> > -/**\n> > - * \\def ARRAY_SIZE(array)\n> > - * \\brief Determine the number of elements in the static array.\n> > - */\n> > -\n> > /**\n> > * \\brief Strip the directory prefix from the path\n> > * \\param[in] path The path to process\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 18220b81af21..51be1dcd7fff 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -7,6 +7,7 @@\n> >\n> > #include \"v4l2_videodevice.h\"\n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -991,13 +992,13 @@ int V4L2VideoDevice::exportBuffers(unsigned int count,\n> >\n> > \tfor (unsigned i = 0; i < count; ++i) {\n> > \t\tstruct v4l2_buffer buf = {};\n> > -\t\tstruct v4l2_plane planes[VIDEO_MAX_PLANES] = {};\n> > +\t\tstd::array planes = {};\n> >\n> > \t\tbuf.index = i;\n> > \t\tbuf.type = bufferType_;\n> > \t\tbuf.memory = memoryType_;\n> > -\t\tbuf.length = ARRAY_SIZE(planes);\n> > -\t\tbuf.m.planes = planes;\n> > +\t\tbuf.length = planes.size();\n> > +\t\tbuf.m.planes = planes.data();\n> >\n> > \t\tret = ioctl(VIDIOC_QUERYBUF, &buf);\n> > \t\tif (ret < 0) {\n> > diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp\n> > index f53042b88720..5bf543c197fa 100644\n> > --- a/test/ipc/unixsocket.cpp\n> > +++ b/test/ipc/unixsocket.cpp\n> > @@ -6,6 +6,7 @@\n> > */\n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -21,7 +22,6 @@\n> > #include \"ipc_unixsocket.h\"\n> > #include \"test.h\"\n> > #include \"thread.h\"\n> > -#include \"utils.h\"\n> >\n> > #define CMD_CLOSE\t0\n> > #define CMD_REVERSE\t1\n> > @@ -303,13 +303,13 @@ protected:\n> > \t\tIPCUnixSocket::Payload message, response;\n> > \t\tint ret;\n> >\n> > -\t\tstatic const char *strings[2] = {\n> > +\t\tstatic constexpr std::array strings = {\n> > \t\t\t\"Foo\",\n> > \t\t\t\"Bar\",\n> > \t\t};\n> > \t\tint fds[2];\n> >\n> > -\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(strings); i++) {\n> > +\t\tfor (unsigned int i = 0; i < strings.size(); i++) {\n> > \t\t\tunsigned int len = strlen(strings[i]);\n> >\n> > \t\t\tfds[i] = open(\"/tmp\", O_TMPFILE | O_RDWR,\n> > @@ -331,7 +331,7 @@ protected:\n> > \t\tif (ret)\n> > \t\t\treturn ret;\n> >\n> > -\t\tfor (unsigned int i = 0; i < ARRAY_SIZE(strings); i++) {\n> > +\t\tfor (unsigned int i = 0; i < strings.size(); i++) {\n> > \t\t\tunsigned int len = strlen(strings[i]);\n> > \t\t\tchar buf[len];\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 8689B60804\n\tfor ;\n\tMon, 20 Jan 2020 17:34:48 +0100 (CET)","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 F0701A62;\n\tMon, 20 Jan 2020 17:34:47 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1579538088;\n\tbh=B0QrQ9qT2KyD2qPmwy201s457537ig4XYnbmk+yjRwE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=A5cQiI5a/ROTjk+IOV440ACClVapMw6FwtkhGpVjqaFN1iv9wReV53C0xQv4afaJv\n\taWnVkNqMEbu1X0CfLPEaNVlGx+BMdqUR0ckx2U8EaOPvhhxjPk3/KeUopBzuOiXfYv\n\tOxb6tAAhdrlnb/LQj3bln1vEd+ZbD+HcqWtDa0WU=","Date":"Mon, 20 Jan 2020 18:34:47 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200120163447.GI20122@pendragon.ideasonboard.com>","References":"<20200120002437.6633-1-laurent.pinchart@ideasonboard.com>\n\t<20200120002437.6633-6-laurent.pinchart@ideasonboard.com>\n\t<20200120132732.c2efqu2zb5a4wguu@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200120132732.c2efqu2zb5a4wguu@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 05/19] libcamera: Replace ARRAY_SIZE\n\twith std::array","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 20 Jan 2020 16:34:48 -0000"}}]