[libcamera-devel] [PATCH 05/19] libcamera: Replace ARRAY_SIZE with std::array
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jan 20 17:34:47 CET 2020
Hi Jacopo,
On Mon, Jan 20, 2020 at 02:27:32PM +0100, Jacopo Mondi wrote:
> On Mon, Jan 20, 2020 at 02:24:23AM +0200, 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.
>
> you know my opinion on std::array already :)
I think you're not alone with that opinion :-) I'm personally not fully
in love with std::array in all cases, but there are clear upsides. I
don't intend in pushing this patch if there's no consensus. I'll send a
v2 that will introduce utils::make_array() to fix one of the issues,
let's see what to do from there.
> Anyway
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > 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"
>
> You should include... Ah no, it's there for no reason already :)
>
> All the rest is good
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Thanks
> j
> >
> > /**
> > * \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 = {
> > "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 = {
> > " 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())
> > 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,
Laurent Pinchart
More information about the libcamera-devel
mailing list