[PATCH v2 17/20] libcamera: libcamera: Formatting improvements
Milan Zamazal
mzamazal at redhat.com
Mon Sep 2 16:43:00 CEST 2024
Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> On Mon, Sep 02, 2024 at 03:43:51PM +0200, Milan Zamazal wrote:
>> Hi Laurent,
>>
>
>> thank you for review.
>>
>> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
>>
>> > Hi Milan,
>> >
>> > Thank you for the patch.
>> >
>> > On Fri, Aug 30, 2024 at 05:27:14PM +0200, Milan Zamazal wrote:
>> >> The LSP autoformatter doesn't like some of the current formatting, let's
>> >> make it happy.
>> >>
>> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> >> ---
>> >> src/libcamera/base/event_dispatcher_poll.cpp | 3 +-
>> >> src/libcamera/camera.cpp | 4 +-
>> >> src/libcamera/controls.cpp | 31 +++----
>> >> src/libcamera/ipa_data_serializer.cpp | 95 ++++++++++----------
>> >> src/libcamera/ipa_module.cpp | 15 ++--
>> >> src/libcamera/orientation.cpp | 16 ++--
>> >> src/libcamera/pipeline_handler.cpp | 5 +-
>> >> src/libcamera/process.cpp | 7 +-
>> >> src/libcamera/sensor/camera_sensor.cpp | 6 +-
>> >> src/libcamera/shared_mem_object.cpp | 4 +-
>> >> src/libcamera/stream.cpp | 6 +-
>> >> 11 files changed, 97 insertions(+), 95 deletions(-)
>> >>
>> >> diff --git a/src/libcamera/base/event_dispatcher_poll.cpp b/src/libcamera/base/event_dispatcher_poll.cpp
>> >> index 86a26f36..288246ff 100644
>> >> --- a/src/libcamera/base/event_dispatcher_poll.cpp
>> >> +++ b/src/libcamera/base/event_dispatcher_poll.cpp
>> >> @@ -5,8 +5,6 @@
>> >> * Poll-based event dispatcher
>> >> */
>> >>
>> >> -#include <libcamera/base/event_dispatcher_poll.h>
>> >> -
>> >
>> > This was done on purpose.
>> >
>> >> #include <chrono>
>> >> #include <iomanip>
>> >> #include <poll.h>
>> >> @@ -15,6 +13,7 @@
>> >> #include <sys/eventfd.h>
>> >> #include <unistd.h>
>> >>
>> >> +#include <libcamera/base/event_dispatcher_poll.h>
>> >> #include <libcamera/base/event_notifier.h>
>> >> #include <libcamera/base/log.h>
>> >> #include <libcamera/base/thread.h>
>> >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>> >> index 88210ff3..69e54439 100644
>> >> --- a/src/libcamera/camera.cpp
>> >> +++ b/src/libcamera/camera.cpp
>> >> @@ -5,7 +5,7 @@
>> >> * Camera device
>> >> */
>> >>
>> >> -#include <libcamera/camera.h>
>> >
>> > Same here.
>> >
>> >> +#include "libcamera/internal/camera.h"
>> >
>> > Moving this include here probably make sense.
>> >
>> >>
>> >> #include <array>
>> >> #include <atomic>
>> >> @@ -13,12 +13,12 @@
>> >> #include <libcamera/base/log.h>
>> >> #include <libcamera/base/thread.h>
>> >>
>> >> +#include <libcamera/camera.h>
>> >> #include <libcamera/color_space.h>
>> >> #include <libcamera/framebuffer_allocator.h>
>> >> #include <libcamera/request.h>
>> >> #include <libcamera/stream.h>
>> >>
>> >> -#include "libcamera/internal/camera.h"
>> >> #include "libcamera/internal/camera_controls.h"
>> >> #include "libcamera/internal/pipeline_handler.h"
>> >> #include "libcamera/internal/request.h"
>> >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
>> >> index 67400797..603e2672 100644
>> >> --- a/src/libcamera/controls.cpp
>> >> +++ b/src/libcamera/controls.cpp
>> >> @@ -5,15 +5,15 @@
>> >> * Control handling
>> >> */
>> >>
>> >> -#include <libcamera/controls.h>
>> >> -
>> >
>> > This is on purpose too.
>> >
>> >> #include <sstream>
>> >> -#include <string>
>> >> #include <string.h>
>> >> +#include <string>
>> >>
>> >> #include <libcamera/base/log.h>
>> >> #include <libcamera/base/utils.h>
>> >>
>> >> +#include <libcamera/controls.h>
>> >> +
>> >> #include "libcamera/internal/control_validator.h"
>> >>
>> >> /**
>> >> @@ -51,15 +51,15 @@ LOG_DEFINE_CATEGORY(Controls)
>> >> namespace {
>> >>
>> >> static constexpr size_t ControlValueSize[] = {
>> >> - [ControlTypeNone] = 0,
>> >> - [ControlTypeBool] = sizeof(bool),
>> >> - [ControlTypeByte] = sizeof(uint8_t),
>> >> - [ControlTypeInteger32] = sizeof(int32_t),
>> >> - [ControlTypeInteger64] = sizeof(int64_t),
>> >> - [ControlTypeFloat] = sizeof(float),
>> >> - [ControlTypeString] = sizeof(char),
>> >> - [ControlTypeRectangle] = sizeof(Rectangle),
>> >> - [ControlTypeSize] = sizeof(Size),
>> >> + [ControlTypeNone] = 0,
>> >> + [ControlTypeBool] = sizeof(bool),
>> >> + [ControlTypeByte] = sizeof(uint8_t),
>> >> + [ControlTypeInteger32] = sizeof(int32_t),
>> >> + [ControlTypeInteger64] = sizeof(int64_t),
>> >> + [ControlTypeFloat] = sizeof(float),
>> >> + [ControlTypeString] = sizeof(char),
>> >> + [ControlTypeRectangle] = sizeof(Rectangle),
>> >> + [ControlTypeSize] = sizeof(Size),
>> >
>> > I prefer keeping the indentation but could live with the change.
>> >
>> >> };
>> >>
>> >> } /* namespace */
>> >> @@ -186,8 +186,8 @@ Span<const uint8_t> ControlValue::data() const
>> >> {
>> >> std::size_t size = numElements_ * ControlValueSize[type_];
>> >> const uint8_t *data = size > sizeof(value_)
>> >> - ? reinterpret_cast<const uint8_t *>(storage_)
>> >> - : reinterpret_cast<const uint8_t *>(&value_);
>> >> + ? reinterpret_cast<const uint8_t *>(storage_)
>> >> + : reinterpret_cast<const uint8_t *>(&value_);
>> >
>> > Nack.
>> >
>> >> return { data, size };
>> >> }
>> >>
>> >> @@ -700,7 +700,8 @@ bool ControlInfoMap::validate()
>> >> * values.
>> >> */
>> >> ControlType rangeType = id->type() == ControlTypeString
>> >> - ? ControlTypeInteger32 : id->type();
>> >> + ? ControlTypeInteger32
>> >> + : id->type();
>> >
>> > Nack.
>> >
>> >> const ControlInfo &info = ctrl.second;
>> >>
>> >> if (info.min().type() != rangeType) {
>> >> diff --git a/src/libcamera/ipa_data_serializer.cpp b/src/libcamera/ipa_data_serializer.cpp
>> >> index f6dd7e6f..67a5726a 100644
>> >> --- a/src/libcamera/ipa_data_serializer.cpp
>> >> +++ b/src/libcamera/ipa_data_serializer.cpp
>> >> @@ -188,52 +188,52 @@ namespace {
>> >>
>> >> #ifndef __DOXYGEN__
>> >>
>> >> -#define DEFINE_POD_SERIALIZER(type) \
>> >> - \
>> >> -template<> \
>> >> -std::tuple<std::vector<uint8_t>, std::vector<SharedFD>> \
>> >> -IPADataSerializer<type>::serialize(const type &data, \
>> >> - [[maybe_unused]] ControlSerializer *cs) \
>> >> -{ \
>> >> - std::vector<uint8_t> dataVec; \
>> >> - dataVec.reserve(sizeof(type)); \
>> >> - appendPOD<type>(dataVec, data); \
>> >> - \
>> >> - return { dataVec, {} }; \
>> >> -} \
>> >> - \
>> >> -template<> \
>> >> -type IPADataSerializer<type>::deserialize(std::vector<uint8_t>::const_iterator dataBegin, \
>> >> - std::vector<uint8_t>::const_iterator dataEnd, \
>> >> - [[maybe_unused]] ControlSerializer *cs) \
>> >> -{ \
>> >> - return readPOD<type>(dataBegin, 0, dataEnd); \
>> >> -} \
>> >> - \
>> >> -template<> \
>> >> -type IPADataSerializer<type>::deserialize(const std::vector<uint8_t> &data, \
>> >> - ControlSerializer *cs) \
>> >> -{ \
>> >> - return deserialize(data.cbegin(), data.end(), cs); \
>> >> -} \
>> >> - \
>> >> -template<> \
>> >> -type IPADataSerializer<type>::deserialize(const std::vector<uint8_t> &data, \
>> >> - [[maybe_unused]] const std::vector<SharedFD> &fds, \
>> >> - ControlSerializer *cs) \
>> >> -{ \
>> >> - return deserialize(data.cbegin(), data.end(), cs); \
>> >> -} \
>> >> - \
>> >> -template<> \
>> >> -type IPADataSerializer<type>::deserialize(std::vector<uint8_t>::const_iterator dataBegin, \
>> >> - std::vector<uint8_t>::const_iterator dataEnd, \
>> >> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsBegin, \
>> >> - [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd, \
>> >> - ControlSerializer *cs) \
>> >> -{ \
>> >> - return deserialize(dataBegin, dataEnd, cs); \
>> >> -}
>> >> +#define DEFINE_POD_SERIALIZER(type) \
>> >> + \
>> >> + template<> \
>> >> + std::tuple<std::vector<uint8_t>, std::vector<SharedFD>> \
>> >> + IPADataSerializer<type>::serialize(const type &data, \
>> >> + [[maybe_unused]] ControlSerializer *cs) \
>> >> + { \
>> >> + std::vector<uint8_t> dataVec; \
>> >> + dataVec.reserve(sizeof(type)); \
>> >> + appendPOD<type>(dataVec, data); \
>> >> + \
>> >> + return { dataVec, {} }; \
>> >> + } \
>> >> + \
>> >> + template<> \
>> >> + type IPADataSerializer<type>::deserialize(std::vector<uint8_t>::const_iterator dataBegin, \
>> >> + std::vector<uint8_t>::const_iterator dataEnd, \
>> >> + [[maybe_unused]] ControlSerializer *cs) \
>> >> + { \
>> >> + return readPOD<type>(dataBegin, 0, dataEnd); \
>> >> + } \
>> >> + \
>> >> + template<> \
>> >> + type IPADataSerializer<type>::deserialize(const std::vector<uint8_t> &data, \
>> >> + ControlSerializer *cs) \
>> >> + { \
>> >> + return deserialize(data.cbegin(), data.end(), cs); \
>> >> + } \
>> >> + \
>> >> + template<> \
>> >> + type IPADataSerializer<type>::deserialize(const std::vector<uint8_t> &data, \
>> >> + [[maybe_unused]] const std::vector<SharedFD> &fds, \
>> >> + ControlSerializer *cs) \
>> >> + { \
>> >> + return deserialize(data.cbegin(), data.end(), cs); \
>> >> + } \
>> >> + \
>> >> + template<> \
>> >> + type IPADataSerializer<type>::deserialize(std::vector<uint8_t>::const_iterator dataBegin, \
>> >> + std::vector<uint8_t>::const_iterator dataEnd, \
>> >> + [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsBegin, \
>> >> + [[maybe_unused]] std::vector<SharedFD>::const_iterator fdsEnd, \
>> >> + ControlSerializer *cs) \
>> >> + { \
>> >> + return deserialize(dataBegin, dataEnd, cs); \
>> >> + }
>> >
>> > Complete nack :-)
>> >
>> >>
>> >> DEFINE_POD_SERIALIZER(bool)
>> >> DEFINE_POD_SERIALIZER(uint8_t)
>> >> @@ -539,7 +539,6 @@ IPADataSerializer<SharedFD>::serialize(const SharedFD &data,
>> >> if (data.isValid())
>> >> fdVec.push_back(data);
>> >>
>> >> -
>> >
>> > Ack.
>> >
>> >> return { dataVec, fdVec };
>> >> }
>> >>
>> >> @@ -606,7 +605,7 @@ IPADataSerializer<FrameBuffer::Plane>::deserialize(std::vector<uint8_t>::const_i
>> >> FrameBuffer::Plane ret;
>> >>
>> >> ret.fd = IPADataSerializer<SharedFD>::deserialize(dataBegin, dataBegin + 4,
>> >> - fdsBegin, fdsBegin + 1);
>> >> + fdsBegin, fdsBegin + 1);
>> >
>> > Ack.
>> >
>> >> ret.offset = readPOD<uint32_t>(dataBegin, 4, dataEnd);
>> >> ret.length = readPOD<uint32_t>(dataBegin, 8, dataEnd);
>> >>
>> >> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
>> >> index 86d88a86..b8b2eb6c 100644
>> >> --- a/src/libcamera/ipa_module.cpp
>> >> +++ b/src/libcamera/ipa_module.cpp
>> >> @@ -50,8 +50,8 @@ typename std::remove_extent_t<T> *elfPointer(Span<const uint8_t> elf,
>> >> if (size > elf.size() || size < objSize)
>> >> return nullptr;
>> >>
>> >> - return reinterpret_cast<typename std::remove_extent_t<T> *>
>> >> - (reinterpret_cast<const char *>(elf.data()) + offset);
>> >> + return reinterpret_cast<typename std::remove_extent_t<T> *>(
>> >> + reinterpret_cast<const char *>(elf.data()) + offset);
>> >
>> > Ack.
>> >
>> >> }
>> >>
>> >> template<typename T>
>> >> @@ -80,21 +80,22 @@ int elfVerifyIdent(Span<const uint8_t> elf)
>> >>
>> >> int a = 1;
>> >> unsigned char endianness = *reinterpret_cast<char *>(&a) == 1
>> >> - ? ELFDATA2LSB : ELFDATA2MSB;
>> >> + ? ELFDATA2LSB
>> >> + : ELFDATA2MSB;
>> >
>> > Nack.
>> >
>> >> if (e_ident[EI_DATA] != endianness)
>> >> return -ENOEXEC;
>> >>
>> >> return 0;
>> >> }
>> >>
>> >> -const ElfW(Shdr) *elfSection(Span<const uint8_t> elf, const ElfW(Ehdr) *eHdr,
>> >> - ElfW(Half) idx)
>> >> +const ElfW(Shdr) * elfSection(Span<const uint8_t> elf, const ElfW(Ehdr) * eHdr,
>> >
>> > Something confuses clang-format clearly.
>> >
>> >> + ElfW(Half) idx)
>> >> {
>> >> if (idx >= eHdr->e_shnum)
>> >> return nullptr;
>> >>
>> >> - off_t offset = eHdr->e_shoff + idx *
>> >> - static_cast<uint32_t>(eHdr->e_shentsize);
>> >> + off_t offset =
>> >> + eHdr->e_shoff + idx * static_cast<uint32_t>(eHdr->e_shentsize);
>> >
>> > Not nicer. We could do
>> >
>> > off_t offset = eHdr->e_shoff
>> > + idx * static_cast<uint32_t>(eHdr->e_shentsize);
>> >
>> > but I suppose clang-format would still not be happy/
>> >
>> >> return elfPointer<const ElfW(Shdr)>(elf, offset);
>> >> }
>> >>
>> >> diff --git a/src/libcamera/orientation.cpp b/src/libcamera/orientation.cpp
>> >> index bf960249..80dea459 100644
>> >> --- a/src/libcamera/orientation.cpp
>> >> +++ b/src/libcamera/orientation.cpp
>> >> @@ -5,10 +5,10 @@
>> >> * Image orientation
>> >> */
>> >>
>> >> -#include <libcamera/orientation.h>
>> >> -
>> >
>> > On purpose.
>> >
>> >> #include <array>
>> >>
>> >> +#include <libcamera/orientation.h>
>> >> +
>> >> /**
>> >> * \file orientation.h
>> >> * \brief Image orientation definition
>> >> @@ -101,10 +101,14 @@ std::ostream &operator<<(std::ostream &out, const Orientation &orientation)
>> >> {
>> >> constexpr std::array<const char *, 9> orientationNames = {
>> >> "", /* Orientation starts counting from 1. */
>> >> - "Rotate0", "Rotate0Mirror",
>> >> - "Rotate180", "Rotate180Mirror",
>> >> - "Rotate90Mirror", "Rotate270",
>> >> - "Rotate270Mirror", "Rotate90",
>> >> + "Rotate0",
>> >> + "Rotate0Mirror",
>> >> + "Rotate180",
>> >> + "Rotate180Mirror",
>> >> + "Rotate90Mirror",
>> >> + "Rotate270",
>> >> + "Rotate270Mirror",
>> >> + "Rotate90",
>> >
>> > I'm OK with this.
>> >
>> >> };
>> >>
>> >> out << orientationNames[static_cast<unsigned int>(orientation)];
>> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>> >> index a63d3503..981c2e64 100644
>> >> --- a/src/libcamera/pipeline_handler.cpp
>> >> +++ b/src/libcamera/pipeline_handler.cpp
>> >> @@ -577,8 +577,9 @@ std::string PipelineHandler::configurationFile(const std::string &subdir,
>> >> << confPath << "'";
>> >> } else {
>> >> /* Else look in the system locations. */
>> >> - confPath = std::string(LIBCAMERA_DATA_DIR)
>> >> - + "/pipeline/" + subdir + '/' + name;
>> >> + confPath =
>> >> + std::string(LIBCAMERA_DATA_DIR) +
>> >> + "/pipeline/" + subdir + '/' + name;
>> >
>> > Looks weird. We could do
>> >
>> > confPath = std::string(LIBCAMERA_DATA_DIR) + "/pipeline/"
>> > + subdir + '/' + name;
>>
>> It seems that what clang-format doesn't like is `+' in front. It
>> re-formats your example as a single line, which is then too long, while
>> it is happy with
>>
>> confPath = std::string(LIBCAMERA_DATA_DIR) + "/pipeline/" +
>> subdir + '/' + name;
>>
>> I'll keep the original version.
>
> I wonder if
> https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignoperands
> could help.
Setting
BreakBeforeBinaryOperators: true
allows starting the line with `+'.
As for the alignment, my clang-format 17.0.6 doesn't know
AlignAfterOperator, which should in theory make exactly what you wrote
above.
>> >> }
>> >>
>> >> ret = stat(confPath.c_str(), &statbuf);
>> >> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
>> >> index c0f4d49f..68fad327 100644
>> >> --- a/src/libcamera/process.cpp
>> >> +++ b/src/libcamera/process.cpp
>> >> @@ -75,7 +75,7 @@ void ProcessManager::sighandler()
>> >> return;
>> >> }
>> >>
>> >> - for (auto it = processes_.begin(); it != processes_.end(); ) {
>> >> + for (auto it = processes_.begin(); it != processes_.end();) {
>> >
>> > I think it hinders readability a bit.
>> >
>> >> Process *process = *it;
>> >>
>> >> int wstatus;
>> >> @@ -188,7 +188,6 @@ const struct sigaction &ProcessManager::oldsa() const
>> >> return oldsa_;
>> >> }
>> >>
>> >> -
>> >
>> > Ack.
>> >
>> >> /**
>> >> * \class Process
>> >> * \brief Process object
>> >> @@ -270,8 +269,8 @@ int Process::start(const std::string &path,
>> >> unsigned int len = args.size();
>> >> argv[0] = path.c_str();
>> >> for (unsigned int i = 0; i < len; i++)
>> >> - argv[i+1] = args[i].c_str();
>> >> - argv[len+1] = nullptr;
>> >> + argv[i + 1] = args[i].c_str();
>> >> + argv[len + 1] = nullptr;
>> >
>> > Ack.
>> >
>> >>
>> >> execv(path.c_str(), (char **)argv);
>> >>
>> >> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp
>> >> index 1382081a..4a990bb9 100644
>> >> --- a/src/libcamera/sensor/camera_sensor.cpp
>> >> +++ b/src/libcamera/sensor/camera_sensor.cpp
>> >> @@ -6,7 +6,6 @@
>> >> */
>> >>
>> >> #include "libcamera/internal/camera_sensor.h"
>> >> -#include "libcamera/internal/media_device.h"
>> >>
>> >> #include <algorithm>
>> >> #include <float.h>
>> >> @@ -14,15 +13,16 @@
>> >> #include <math.h>
>> >> #include <string.h>
>> >>
>> >> +#include <libcamera/base/utils.h>
>> >> +
>> >> #include <libcamera/camera.h>
>> >> #include <libcamera/orientation.h>
>> >> #include <libcamera/property_ids.h>
>> >>
>> >> -#include <libcamera/base/utils.h>
>> >> -
>> >> #include "libcamera/internal/bayer_format.h"
>> >> #include "libcamera/internal/camera_lens.h"
>> >> #include "libcamera/internal/camera_sensor_properties.h"
>> >> +#include "libcamera/internal/media_device.h"
>> >> #include "libcamera/internal/sysfs.h"
>> >
>> > Ack.
>> >
>> >>
>> >> /**
>> >> diff --git a/src/libcamera/shared_mem_object.cpp b/src/libcamera/shared_mem_object.cpp
>> >> index 65b53919..d9b61d37 100644
>> >> --- a/src/libcamera/shared_mem_object.cpp
>> >> +++ b/src/libcamera/shared_mem_object.cpp
>> >> @@ -57,8 +57,8 @@ SharedMem::SharedMem() = default;
>> >> */
>> >> SharedMem::SharedMem(const std::string &name, std::size_t size)
>> >> {
>> >> - UniqueFD memfd = MemFd::create(name.c_str(), size, MemFd::Seal::Shrink |
>> >> - MemFd::Seal::Grow);
>> >> + UniqueFD memfd = MemFd::create(name.c_str(), size,
>> >> + MemFd::Seal::Shrink | MemFd::Seal::Grow);
>> >
>> > I'm OK with that.
>> >
>> >> if (!memfd.isValid())
>> >> return;
>> >>
>> >> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
>> >> index e70688f6..00b15608 100644
>> >> --- a/src/libcamera/stream.cpp
>> >> +++ b/src/libcamera/stream.cpp
>> >> @@ -5,17 +5,15 @@
>> >> * Video stream for a Camera
>> >> */
>> >>
>> >> -#include <libcamera/stream.h>
>> >> -
>> >
>> > That was done on purpose.
>> >
>> >> #include <algorithm>
>> >> #include <array>
>> >> #include <limits.h>
>> >>
>> >> -#include <libcamera/request.h>
>> >> -
>> >> #include <libcamera/base/log.h>
>> >> #include <libcamera/base/utils.h>
>> >>
>> >> +#include <libcamera/request.h>
>> >
>> > Ack.
>> >
>> >> +#include <libcamera/stream.h>
>> >>
>> >> /**
>> >> * \file stream.h
More information about the libcamera-devel
mailing list