[libcamera-devel] [PATCH] libcamera: Replace ARRAY_SIZE() with std::size()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Dec 14 14:43:03 CET 2020
Hi Jacopo,
On Mon, Dec 14, 2020 at 08:50:38AM +0100, Jacopo Mondi wrote:
> On Sat, Dec 12, 2020 at 01:23:19AM +0200, Laurent Pinchart wrote:
> > C++17 has a std::size() function that returns the size of a C-style
> > array. Use it instead of the custom ARRAY_SIZE macro.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > include/libcamera/internal/utils.h | 2 --
> > src/ipa/raspberrypi/raspberrypi.cpp | 4 ++--
> > src/libcamera/camera.cpp | 8 ++++----
> > src/libcamera/log.cpp | 7 ++++---
> > src/libcamera/utils.cpp | 5 -----
> > src/libcamera/v4l2_videodevice.cpp | 6 +++---
> > test/ipc/unixsocket.cpp | 6 +++---
> > 7 files changed, 16 insertions(+), 22 deletions(-)
> >
> > diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h
> > index ebb2c4038e19..f08134afb5ba 100644
> > --- a/include/libcamera/internal/utils.h
> > +++ b/include/libcamera/internal/utils.h
> > @@ -17,8 +17,6 @@
> > #include <sys/time.h>
> > #include <vector>
> >
> > -#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
> > -
> > #ifndef __DOXYGEN__
> >
> > /* uClibc and uClibc-ng don't provide O_TMPFILE */
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 801400214c3a..a7439badb7dc 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -6,6 +6,7 @@
> > */
> >
> > #include <algorithm>
> > +#include <array>
> > #include <fcntl.h>
> > #include <math.h>
> > #include <stdint.h>
> > @@ -27,7 +28,6 @@
> > #include "libcamera/internal/buffer.h"
> > #include "libcamera/internal/camera_sensor.h"
> > #include "libcamera/internal/log.h"
> > -#include "libcamera/internal/utils.h"
> >
> > #include <linux/bcm2835-isp.h>
> >
> > @@ -1092,7 +1092,7 @@ void IPARPi::applyLS(const struct AlscStatus *lsStatus, ControlList &ctrls)
> > * Choose smallest cell size that won't exceed 63x48 cells.
> > */
> > const int cellSizes[] = { 16, 32, 64, 128, 256 };
> > - unsigned int numCells = ARRAY_SIZE(cellSizes);
> > + unsigned int numCells = std::size(cellSizes);
>
> I wonder if replacing cellSizes[] with a const std::array and use
> std::array::size() wouldn't be more C++-ish.
The trouble with std::array() is that you need to declare the array size
explicitly. This would become
const std::array<int, 5> cellSizes{ 16, 32, 64, 128, 256 };
An experimental std::make_array() has been proposed (see
https://en.cppreference.com/w/cpp/experimental/make_array) to overcome
that limitation. It has been dropped in favour of std::to_array() (see
https://en.cppreference.com/w/cpp/container/array/to_array) that has
been merged in C++20. The code would then become
const auto cellSizes = std::to_array<int>({ 16, 32, 64, 128, 256 });
We could possibly implement this function in the utils namespace (that
would need to be double-checked though, as it may depend template
deduction rules that have been updated in C++20). I'm not sure if the
end result is better though.
> The change itself is fine though
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > unsigned int i, w, h, cellSize;
> > for (i = 0; i < numCells; i++) {
> > cellSize = cellSizes[i];
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 3579ba96b6ea..c1de1fee701a 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -7,6 +7,7 @@
> >
> > #include <libcamera/camera.h>
> >
> > +#include <array>
> > #include <atomic>
> > #include <iomanip>
> >
> > @@ -17,7 +18,6 @@
> > #include "libcamera/internal/log.h"
> > #include "libcamera/internal/pipeline_handler.h"
> > #include "libcamera/internal/thread.h"
> > -#include "libcamera/internal/utils.h"
> >
> > /**
> > * \file camera.h
> > @@ -393,7 +393,7 @@ int Camera::Private::isAccessAllowed(State state, bool allowDisconnected) const
> > if (currentState == state)
> > return 0;
> >
> > - ASSERT(static_cast<unsigned int>(state) < ARRAY_SIZE(camera_state_names));
> > + ASSERT(static_cast<unsigned int>(state) < std::size(camera_state_names));
> >
> > LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState]
> > << " state trying operation requiring state "
> > @@ -412,8 +412,8 @@ int Camera::Private::isAccessAllowed(State low, State high,
> > if (currentState >= low && currentState <= high)
> > return 0;
> >
> > - 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) < std::size(camera_state_names) &&
> > + static_cast<unsigned int>(high) < std::size(camera_state_names));
> >
> > LOG(Camera, Debug) << "Camera in " << camera_state_names[currentState]
> > << " state trying operation requiring state between "
> > diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> > index 180eb97ba664..45c7c2d24652 100644
> > --- a/src/libcamera/log.cpp
> > +++ b/src/libcamera/log.cpp
> > @@ -7,6 +7,7 @@
> >
> > #include "libcamera/internal/log.h"
> >
> > +#include <array>
> > #if HAVE_BACKTRACE
> > #include <execinfo.h>
> > #endif
> > @@ -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) < std::size(names))
> > return names[severity];
> > else
> > return "UNKWN";
> > @@ -406,7 +407,7 @@ void Logger::backtrace()
> > return;
> >
> > void *buffer[32];
> > - int num_entries = ::backtrace(buffer, ARRAY_SIZE(buffer));
> > + int num_entries = ::backtrace(buffer, std::size(buffer));
> > char **strings = backtrace_symbols(buffer, num_entries);
> > if (!strings)
> > return;
> > @@ -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 < std::size(names); ++i) {
> > if (names[i] == level) {
> > severity = i;
> > break;
> > diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> > index da85c9c24340..e90375ae115c 100644
> > --- a/src/libcamera/utils.cpp
> > +++ b/src/libcamera/utils.cpp
> > @@ -31,11 +31,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 e2b582842a9b..baf683d6d985 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -7,6 +7,7 @@
> >
> > #include "libcamera/internal/v4l2_videodevice.h"
> >
> > +#include <array>
> > #include <fcntl.h>
> > #include <iomanip>
> > #include <sstream>
> > @@ -26,7 +27,6 @@
> > #include "libcamera/internal/log.h"
> > #include "libcamera/internal/media_device.h"
> > #include "libcamera/internal/media_object.h"
> > -#include "libcamera/internal/utils.h"
> >
> > /**
> > * \file v4l2_videodevice.h
> > @@ -860,7 +860,7 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)
> > pix->num_planes = format->planesCount;
> > pix->field = V4L2_FIELD_NONE;
> >
> > - ASSERT(pix->num_planes <= ARRAY_SIZE(pix->plane_fmt));
> > + ASSERT(pix->num_planes <= std::size(pix->plane_fmt));
> >
> > for (unsigned int i = 0; i < pix->num_planes; ++i) {
> > pix->plane_fmt[i].bytesperline = format->planes[i].bpl;
> > @@ -1255,7 +1255,7 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> > buf.index = index;
> > buf.type = bufferType_;
> > buf.memory = V4L2_MEMORY_MMAP;
> > - buf.length = ARRAY_SIZE(v4l2Planes);
> > + buf.length = std::size(v4l2Planes);
> > buf.m.planes = v4l2Planes;
> >
> > int ret = ioctl(VIDIOC_QUERYBUF, &buf);
> > diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp
> > index 19a1d7dd8a2d..80157b342795 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>
> > @@ -19,7 +20,6 @@
> > #include "libcamera/internal/ipc_unixsocket.h"
> > #include "libcamera/internal/thread.h"
> > #include "libcamera/internal/timer.h"
> > -#include "libcamera/internal/utils.h"
> >
> > #include "test.h"
> >
> > @@ -311,7 +311,7 @@ protected:
> > };
> > int fds[2];
> >
> > - for (unsigned int i = 0; i < ARRAY_SIZE(strings); i++) {
> > + for (unsigned int i = 0; i < std::size(strings); i++) {
> > unsigned int len = strlen(strings[i]);
> >
> > fds[i] = open("/tmp", O_TMPFILE | O_RDWR,
> > @@ -333,7 +333,7 @@ protected:
> > if (ret)
> > return ret;
> >
> > - for (unsigned int i = 0; i < ARRAY_SIZE(strings); i++) {
> > + for (unsigned int i = 0; i < std::size(strings); i++) {
> > unsigned int len = strlen(strings[i]);
> > char buf[len];
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list