[libcamera-devel] [PATCH v2 4/5] apply clang-format style
Christian Rauch
Rauch.Christian at gmx.de
Wed Apr 6 01:30:10 CEST 2022
Hi Laurent,
I submitted this code-style-only patch as part of my patch series, as I
assumed that the libcamera project wants to apply the clang-format style
over time to the complete code base. In a future version, I will simply
drop this patch and only format the actual changes that I am submitting.
Best,
Christian
Am 05.04.22 um 17:36 schrieb Laurent Pinchart:
> Hi Christian,
>
> Thank you for the patch.
>
> On Tue, Apr 05, 2022 at 01:42:14AM +0100, Christian Rauch via libcamera-devel wrote:
>> Signed-off-by: Christian Rauch <Rauch.Christian at gmx.de>
>> ---
>> include/libcamera/base/span.h | 45 +++---
>> src/ipa/raspberrypi/raspberrypi.cpp | 3 +-
>> .../pipeline/raspberrypi/raspberrypi.cpp | 34 +++--
>> src/qcam/dng_writer.cpp | 140 +++++++++---------
>> test/span.cpp | 4 +-
>> 5 files changed, 116 insertions(+), 110 deletions(-)
>>
>> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h
>> index bff4c115..b2fdd5fb 100644
>> --- a/include/libcamera/base/span.h
>> +++ b/include/libcamera/base/span.h
>> @@ -124,7 +124,7 @@ public:
>> constexpr Span(element_type (&arr)[N],
>> std::enable_if_t<std::is_convertible<std::remove_pointer_t<decltype(utils::data(arr))> (*)[],
>> element_type (*)[]>::value &&
>> - N == Extent,
>> + N == Extent,
>
> clang-format is unfortunately not able to handle all the details of the
> coding style, we can't express the alignment we would like here. I find
> the existing alignment more readable (provided complex templates can
> even be considered as readable :-)).
>
> Same for most locations below, I've only kept the chunks for which I
> have specific comments.
>
>> std::nullptr_t> = nullptr) noexcept
>> : data_(arr)
>> {
>
> [snip]
>
>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>> index 1bf4e270..926b3185 100644
>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> @@ -22,11 +22,12 @@
>> #include <libcamera/control_ids.h>
>> #include <libcamera/controls.h>
>> #include <libcamera/framebuffer.h>
>> +#include <libcamera/request.h>
>> +
>> #include <libcamera/ipa/ipa_interface.h>
>> #include <libcamera/ipa/ipa_module_info.h>
>> #include <libcamera/ipa/raspberrypi.h>
>> #include <libcamera/ipa/raspberrypi_ipa_interface.h>
>> -#include <libcamera/request.h>
>
> This one is a good change.
>
>>
>> #include "libcamera/internal/mapped_framebuffer.h"
>>
>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> index 8fd79be6..34d9f4c4 100644
>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> @@ -10,26 +10,26 @@
>> #include <fcntl.h>
>> #include <memory>
>> #include <mutex>
>> -#include <queue>
>
> That's a false positive, likely caused by the rule that tries to handle
> Qt headers.
>
>> #include <unordered_set>
>> #include <utility>
>>
>> +#include <linux/bcm2835-isp.h>
>> +#include <linux/media-bus-format.h>
>> +#include <linux/videodev2.h>
>> +
>> #include <libcamera/base/shared_fd.h>
>> #include <libcamera/base/utils.h>
>>
>> #include <libcamera/camera.h>
>> #include <libcamera/control_ids.h>
>> #include <libcamera/formats.h>
>> -#include <libcamera/ipa/raspberrypi.h>
>> -#include <libcamera/ipa/raspberrypi_ipa_interface.h>
>> -#include <libcamera/ipa/raspberrypi_ipa_proxy.h>
>> #include <libcamera/logging.h>
>> #include <libcamera/property_ids.h>
>> #include <libcamera/request.h>
>>
>> -#include <linux/bcm2835-isp.h>
>> -#include <linux/media-bus-format.h>
>> -#include <linux/videodev2.h>
>> +#include <libcamera/ipa/raspberrypi.h>
>> +#include <libcamera/ipa/raspberrypi_ipa_interface.h>
>> +#include <libcamera/ipa/raspberrypi_ipa_proxy.h>
>
> The rest is good.
>
>>
>> #include "libcamera/internal/bayer_format.h"
>> #include "libcamera/internal/camera.h"
>
> [snip]
>
>> @@ -544,7 +553,6 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>>
>> cfg.stride = format.planes[0].bpl;
>> cfg.frameSize = format.planes[0].size;
>> -
>
> This is good too.
>
>> }
>>
>> return status;
>> @@ -651,8 +659,8 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
>> PixelFormat pf = mbusCodeToPixelFormat(format.first,
>> BayerFormat::Packing::CSI2);
>> if (pf.isValid())
>> - deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf),
>> - std::forward_as_tuple(format.second.begin(), format.second.end()));
>> + deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf),
>> + std::forward_as_tuple(format.second.begin(), format.second.end()));
>
> While at it, you can write
>
> deviceFormats.emplace(std::piecewise_construct,
> std::forward_as_tuple(pf),
> std::forward_as_tuple(format.second.begin(),
> format.second.end()));
>
>> }
>> } else {
>> /*
>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
>> index 34c8df5a..a7dd30f8 100644
>> --- a/src/qcam/dng_writer.cpp
>> +++ b/src/qcam/dng_writer.cpp
>> @@ -11,12 +11,12 @@
>> #include <iostream>
>> #include <map>
>>
>> -#include <tiffio.h>
>> -
>> #include <libcamera/control_ids.h>
>> #include <libcamera/formats.h>
>> #include <libcamera/property_ids.h>
>>
>> +#include <tiffio.h>
>> +
>
> Fine with me.
>
>> using namespace libcamera;
>>
>> enum CFAPatternColour : uint8_t {
>> @@ -201,7 +201,7 @@ void packScanlineIPU3(void *output, const void *input, unsigned int width)
>> if (++x >= width)
>> return;
>>
>> - *out++ = (in[4] & 0xff) << 8 | (in[3] & 0xc0) << 0;
>> + *out++ = (in[4] & 0xff) << 8 | (in[3] & 0xc0) << 0;
>
> Looks good too.
>
>> if (++x >= width)
>> return;
>>
>
> [snip]
>
>> @@ -254,14 +253,14 @@ void thumbScanlineIPU3([[maybe_unused]] const FormatInfo &info, void *output,
>> break;
>> case 2:
>> val1 = (in[3] & 0x3f) << 10 | (in[2] & 0xf0) << 2;
>> - val2 = (in[4] & 0xff) << 8 | (in[3] & 0xc0) << 0;
>> + val2 = (in[4] & 0xff) << 8 | (in[3] & 0xc0) << 0;
>
> This was done on purpose for alignment reasons, I'd keep it as-is. Same
> below.
>
>> val3 = (in[stride + 3] & 0x3f) << 10 | (in[stride + 2] & 0xf0) << 2;
>> - val4 = (in[stride + 4] & 0xff) << 8 | (in[stride + 3] & 0xc0) << 0;
>> + val4 = (in[stride + 4] & 0xff) << 8 | (in[stride + 3] & 0xc0) << 0;
>> break;
>> case 3:
>> - val1 = (in[4] & 0xff) << 8 | (in[3] & 0xc0) << 0;
>> + val1 = (in[4] & 0xff) << 8 | (in[3] & 0xc0) << 0;
>> val2 = (in[6] & 0x03) << 14 | (in[5] & 0xff) << 6;
>> - val3 = (in[stride + 4] & 0xff) << 8 | (in[stride + 3] & 0xc0) << 0;
>> + val3 = (in[stride + 4] & 0xff) << 8 | (in[stride + 3] & 0xc0) << 0;
>> val4 = (in[stride + 6] & 0x03) << 14 | (in[stride + 5] & 0xff) << 6;
>> break;
>> }
>
> [snip]
>
>> diff --git a/test/span.cpp b/test/span.cpp
>> index abf3a5d6..3c3f6937 100644
>> --- a/test/span.cpp
>> +++ b/test/span.cpp
>> @@ -9,12 +9,12 @@
>> * Include first to ensure the header is self-contained, as there's no span.cpp
>> * in libcamera.
>> */
>> -#include <libcamera/base/span.h>
>> -
>
> See the comment above :-) This change should be dropped.
>
>> #include <array>
>> #include <iostream>
>> #include <vector>
>>
>> +#include <libcamera/base/span.h>
>> +
>> #include "test.h"
>>
>> using namespace std;
>
> Can you take these comments into account, and drop the changes that have
> been [snip]'ed ?
>
More information about the libcamera-devel
mailing list