[PATCH] libcamera: formatting: Avoid spaces in for loops without expression

[Milan Zamazal]

Hi Stanislaw,

Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com> writes:

> Hi,
>
> On Tue, Feb 25, 2025 at 11:14:07PM +0000, Kieran Bingham wrote:
>> Quoting Laurent Pinchart (2025-02-25 21:22:03)
>> > Hi Milan,
>> > 
>> > Thank you for the patch.
>> > 
>> > On Tue, Feb 25, 2025 at 04:28:06PM +0100, Milan Zamazal wrote:
>> > > The clang formatter removes spaces in places of empty expressions in for
>> > > loops.  For example, it changes
>> > > 
>> > >   for (init; condition; )
>> > > 
>> > > to
>> > > 
>> > >   for (init; condition;)
>> > > 
>> > > libcamera currently uses both the styles and we should use only one of
>> > > them for consistency.  Since there is apparently no option to override
>> > > the formatter behavior (see
>> > > https://clang.llvm.org/docs/ClangFormatStyleOptions.html), let's remove
>> > > the extra spaces to make the formatter happy.
>> > > 
>> > > Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> > 
>> > It's not my favourite style, but I'm OK with it given the inconvenience
>> > of making it a clang-format exception.
>> 
>> I remember we discussed this quite some time ago, but I'm fine with
>> making the formatter happy - that makes all our lives easier.
>
> BTW I had formatter issue when editing src/libcamera/v4l2_device.cpp 
> It get me changes like this:
>
> @@ -482,11 +482,12 @@ int V4L2Device::setFrameStartEnabled(bool enable)
>  	if (frameStartEnabled_ == enable)
>  		return 0;
>  
> -	struct v4l2_event_subscription event{};
> +	struct v4l2_event_subscription event {
> +	};

My clang formatter (19.1.7) is fine with the original version.

>  	event.type = V4L2_EVENT_FRAME_SYNC;
>  
>  	unsigned long request = enable ? VIDIOC_SUBSCRIBE_EVENT
> -			      : VIDIOC_UNSUBSCRIBE_EVENT;
> +				       : VIDIOC_UNSUBSCRIBE_EVENT;

This makes sense IMHO.

>  	int ret = ioctl(request, &event);
>  	if (enable && ret)
>  		return ret;
> @@ -828,7 +829,8 @@ void V4L2Device::updateControls(ControlList *ctrls,
>   */
>  void V4L2Device::eventAvailable()
>  {
> -	struct v4l2_event event{};
> +	struct v4l2_event event {
> +	};
>  	int ret = ioctl(VIDIOC_DQEVENT, &event);
>  	if (ret < 0) {
>  		LOG(V4L2, Error)
> @@ -850,11 +852,7 @@ void V4L2Device::eventAvailable()
>  
>  static const std::map<uint32_t, ColorSpace> v4l2ToColorSpace = {
>  	{ V4L2_COLORSPACE_RAW, ColorSpace::Raw },
> -	{ V4L2_COLORSPACE_SRGB, {
> -		ColorSpace::Primaries::Rec709,
> -		ColorSpace::TransferFunction::Srgb,
> -		ColorSpace::YcbcrEncoding::Rec601,
> -		ColorSpace::Range::Limited } },
> +	{ V4L2_COLORSPACE_SRGB, { ColorSpace::Primaries::Rec709, ColorSpace::TransferFunction::Srgb, ColorSpace::YcbcrEncoding::Rec601, ColorSpace::Range::Limited } },

Generally, the formatter apparently prefers putting the initial brace on
the next line.  In this case, this works to avoid the long line:

	{ V4L2_COLORSPACE_SRGB,
	  { ColorSpace::Primaries::Rec709,
	    ColorSpace::TransferFunction::Srgb,
	    ColorSpace::YcbcrEncoding::Rec601,
	    ColorSpace::Range::Limited } },

But I'm not sure this is aligned with what the maintainers like.

>  	{ V4L2_COLORSPACE_JPEG, ColorSpace::Sycc },
>  	{ V4L2_COLORSPACE_SMPTE170M, ColorSpace::Smpte170m },
>  	{ V4L2_COLORSPACE_REC709, ColorSpace::Rec709 },
>
>
> What looks fine for me. I think I can post this patch.
>
> But some other auto-format changes does not look that good.
>
> For example in src/libcamera/v4l2_subdevice.cpp
> I have bunch of hunks like this 
>
>  	{ MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE, {
> -		.name = "RGB444_2X8_PADHI_BE",
> -		.code = MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE,
> -		.type = MediaBusFormatInfo::Type::Image,
> -		.bitsPerPixel = 16,
> -		.colourEncoding = PixelFormatInfo::ColourEncodingRGB,
> -	} },
> +						     .name = "RGB444_2X8_PADHI_BE",
> +						     .code = MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE,
> +						     .type = MediaBusFormatInfo::Type::Image,
> +						     .bitsPerPixel = 16,
> +						     .colourEncoding = PixelFormatInfo::ColourEncodingRGB,
> +					     } },

Again, with the brace on the next line, it looks better:

	{ MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE,
	  {
		  .name = "RGB444_2X8_PADHI_BE",
		  .code = MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE,
		  .type = MediaBusFormatInfo::Type::Image,
		  .bitsPerPixel = 16,
		  .colourEncoding = PixelFormatInfo::ColourEncodingRGB,
	  } },

> and one such change
>
> -	for (unsigned int index = 0; ; index++) {
> +	for (unsigned int index = 0;; index++) {
>
> what looks less convenient for me and perhaps is 
> candidate for changing .clang-format rule if possible.

I personally mostly stopped caring about formatting after I entered the
corporate world -- it's impossible to satisfy everybody and in the end
result the code is unreadable for everybody but those that define the
rules :-).  I use Emacs means to change the code presentation on the
screen to avoid the worst offenders and learned to live with that.  An
ideal situation is when a language provides its own canonical tools (see
Rust, Go, ...) because it saves us from all the discussions.

The only really annoying part is when the formatter constantly changes
edited sources and I have to discard the changes all the time.  So I
welcome any changes (whether in .clang-format or in the source files)
that make the formatter and the maintainers more compatible.

> Regards
> Stanislaw
>