[libcamera-devel] [PATCH 05/19] libcamera: Replace ARRAY_SIZE with std::array
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jan 20 14:36:52 CET 2020
Hi Laurent,
On 20/01/2020 00:24, Laurent Pinchart wrote:
> Replace the C-style arrays with std::array wherever the ARRAY_SIZE macro
> is used, removing the need for the macro completely. std::array combines
> the performance and accessibility of C-style arrays with the benefits of
> a standard container, which is shown here through the ability to carry
> its size.
>
"... at the expense of no longer being able to determine the array size
at compile time."
Sometimes it's nice to be able to just add to an array and know that the
addition will be picked up correctly. (Adding to static lists etc).
Ho hum though ... this is probably progress and forces the developer to
think more about the allocations appropriately.
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> src/libcamera/camera.cpp | 10 +++++-----
> src/libcamera/include/utils.h | 2 --
> src/libcamera/ipa_module.cpp | 8 ++++----
> src/libcamera/log.cpp | 15 ++++++++-------
> src/libcamera/utils.cpp | 5 -----
> src/libcamera/v4l2_videodevice.cpp | 7 ++++---
> test/ipc/unixsocket.cpp | 8 ++++----
> 7 files changed, 25 insertions(+), 30 deletions(-)
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 79a5f994f9bb..3385c08778b8 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -7,6 +7,7 @@
>
> #include <libcamera/camera.h>
>
> +#include <array>
> #include <iomanip>
>
> #include <libcamera/framebuffer_allocator.h>
> @@ -15,7 +16,6 @@
>
> #include "log.h"
> #include "pipeline_handler.h"
> -#include "utils.h"
>
> /**
> * \file camera.h
> @@ -404,7 +404,7 @@ Camera::~Camera()
> LOG(Camera, Error) << "Removing camera while still in use";
> }
>
> -static const char *const camera_state_names[] = {
> +static constexpr std::array<const char *, 4> camera_state_names = {
> "Available",
> "Acquired",
> "Configured",
> @@ -416,8 +416,8 @@ bool Camera::stateBetween(State low, State high) const
> if (state_ >= low && state_ <= high)
> return true;
>
> - ASSERT(static_cast<unsigned int>(low) < ARRAY_SIZE(camera_state_names) &&
> - static_cast<unsigned int>(high) < ARRAY_SIZE(camera_state_names));
> + ASSERT(static_cast<unsigned int>(low) < camera_state_names.size() &&
> + static_cast<unsigned int>(high) < camera_state_names.size());
>
> LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
> << " state trying operation requiring state between "
> @@ -432,7 +432,7 @@ bool Camera::stateIs(State state) const
> if (state_ == state)
> return true;
>
> - ASSERT(static_cast<unsigned int>(state) < ARRAY_SIZE(camera_state_names));
> + ASSERT(static_cast<unsigned int>(state) < camera_state_names.size());
>
> LOG(Camera, Debug) << "Camera in " << camera_state_names[state_]
> << " state trying operation requiring state "
> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> index e467eb21c518..f55c52f72bd5 100644
> --- a/src/libcamera/include/utils.h
> +++ b/src/libcamera/include/utils.h
> @@ -15,8 +15,6 @@
> #include <string.h>
> #include <sys/time.h>
>
> -#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
> -
> #ifndef __DOXYGEN__
>
> /* uClibc and uClibc-ng don't provide O_TMPFILE */
> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> index 2c355ea8b5e5..9c927a62b308 100644
> --- a/src/libcamera/ipa_module.cpp
> +++ b/src/libcamera/ipa_module.cpp
> @@ -22,7 +22,6 @@
>
> #include "log.h"
> #include "pipeline_handler.h"
> -#include "utils.h"
>
> /**
> * \file ipa_module.h
> @@ -480,7 +479,7 @@ bool IPAModule::match(PipelineHandler *pipe,
> */
> bool IPAModule::isOpenSource() const
> {
> - static const char *osLicenses[] = {
> + static constexpr std::array<const char *, 8> osLicenses = {
Oh, so we can't do compile time determination of the size of arrays,
We must codify the size in advance?
That feels a bit like we're losing a feature (or convenience) ...
Will the compiler warn(or error) if the given size does not match the
number of elements given?
I guess it might happily over-allocate and prevent under-allocation?
> "GPL-2.0-only",
> "GPL-2.0-or-later",
> "GPL-3.0-only",
> @@ -491,9 +490,10 @@ bool IPAModule::isOpenSource() const
> "LGPL-3.0-or-later",
> };
>
> - for (unsigned int i = 0; i < ARRAY_SIZE(osLicenses); i++)
> - if (!strcmp(osLicenses[i], info_.license))
> + for (const char *license : osLicenses) {
> + if (!strcmp(license, info_.license))
> return true;
> + }
>
> return false;
> }
> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> index 1dac4666b435..ef0b81f77131 100644
> --- a/src/libcamera/log.cpp
> +++ b/src/libcamera/log.cpp
> @@ -7,6 +7,7 @@
>
> #include "log.h"
>
> +#include <array>
> #if HAVE_BACKTRACE
> #include <execinfo.h>
> #endif
> @@ -83,7 +84,7 @@ static int log_severity_to_syslog(LogSeverity severity)
>
> static const char *log_severity_name(LogSeverity severity)
> {
> - static const char *const names[] = {
> + static constexpr std::array<const char *, 5> names = {
Having to explicitly set the sizes seems like we're doing the job of the
compiler...
> " DBG",
> " INFO",
> " WARN",
> @@ -91,7 +92,7 @@ static const char *log_severity_name(LogSeverity severity)
> "FATAL",
> };
>
> - if (static_cast<unsigned int>(severity) < ARRAY_SIZE(names))
> + if (static_cast<unsigned int>(severity) < names.size())
But this reads nicely otherwise...
> return names[severity];
> else
> return "UNKWN";
> @@ -405,9 +406,9 @@ void Logger::backtrace()
> if (!output)
> return;
>
> - void *buffer[32];
> - int num_entries = ::backtrace(buffer, ARRAY_SIZE(buffer));
> - char **strings = backtrace_symbols(buffer, num_entries);
> + std::array<void *, 32> buffer;
> + int num_entries = ::backtrace(buffer.data(), buffer.size());
> + char **strings = backtrace_symbols(buffer.data(), num_entries);
> if (!strings)
> return;
>
> @@ -603,7 +604,7 @@ void Logger::parseLogLevels()
> */
> LogSeverity Logger::parseLogLevel(const std::string &level)
> {
> - static const char *const names[] = {
> + static constexpr std::array<const char *, 5> names = {
> "DEBUG",
> "INFO",
> "WARN",
> @@ -620,7 +621,7 @@ LogSeverity Logger::parseLogLevel(const std::string &level)
> severity = LogInvalid;
> } else {
> severity = LogInvalid;
> - for (unsigned int i = 0; i < ARRAY_SIZE(names); ++i) {
> + for (unsigned int i = 0; i < names.size(); ++i) {
> if (names[i] == level) {
> severity = i;
> break;
> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> index 4beffdab5eb6..74576797ee77 100644
> --- a/src/libcamera/utils.cpp
> +++ b/src/libcamera/utils.cpp
> @@ -22,11 +22,6 @@ namespace libcamera {
>
> namespace utils {
>
> -/**
> - * \def ARRAY_SIZE(array)
> - * \brief Determine the number of elements in the static array.
> - */
> -
> /**
> * \brief Strip the directory prefix from the path
> * \param[in] path The path to process
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 18220b81af21..51be1dcd7fff 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -7,6 +7,7 @@
>
> #include "v4l2_videodevice.h"
>
> +#include <array>
> #include <fcntl.h>
> #include <iomanip>
> #include <sstream>
> @@ -991,13 +992,13 @@ int V4L2VideoDevice::exportBuffers(unsigned int count,
>
> for (unsigned i = 0; i < count; ++i) {
> struct v4l2_buffer buf = {};
> - struct v4l2_plane planes[VIDEO_MAX_PLANES] = {};
> + std::array<struct v4l2_plane, VIDEO_MAX_PLANES> planes = {};
>
> buf.index = i;
> buf.type = bufferType_;
> buf.memory = memoryType_;
> - buf.length = ARRAY_SIZE(planes);
> - buf.m.planes = planes;
> + buf.length = planes.size();
> + buf.m.planes = planes.data();
>
> ret = ioctl(VIDIOC_QUERYBUF, &buf);
> if (ret < 0) {
> diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp
> index f53042b88720..5bf543c197fa 100644
> --- a/test/ipc/unixsocket.cpp
> +++ b/test/ipc/unixsocket.cpp
> @@ -6,6 +6,7 @@
> */
>
> #include <algorithm>
> +#include <array>
> #include <fcntl.h>
> #include <iostream>
> #include <stdlib.h>
> @@ -21,7 +22,6 @@
> #include "ipc_unixsocket.h"
> #include "test.h"
> #include "thread.h"
> -#include "utils.h"
>
> #define CMD_CLOSE 0
> #define CMD_REVERSE 1
> @@ -303,13 +303,13 @@ protected:
> IPCUnixSocket::Payload message, response;
> int ret;
>
> - static const char *strings[2] = {
> + static constexpr std::array<const char *, 2> strings = {
> "Foo",
> "Bar",
> };
> int fds[2];
>
> - for (unsigned int i = 0; i < ARRAY_SIZE(strings); i++) {
> + for (unsigned int i = 0; i < strings.size(); i++) {
> unsigned int len = strlen(strings[i]);
>
> fds[i] = open("/tmp", O_TMPFILE | O_RDWR,
> @@ -331,7 +331,7 @@ protected:
> if (ret)
> return ret;
>
> - for (unsigned int i = 0; i < ARRAY_SIZE(strings); i++) {
> + for (unsigned int i = 0; i < strings.size(); i++) {
> unsigned int len = strlen(strings[i]);
> char buf[len];
>
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list