[libcamera-devel] [PATCH v2] gstreamer: Implement element EOS handling

[Laurent Pinchart]

On Fri, Nov 10, 2023 at 10:15:25PM +0000, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2023. november 10., péntek 17:29 keltezéssel, Laurent Pinchart via libcamera-devel írta:
> 
> > [...]
> > > +	if (GstEvent *event = self->pending_eos.exchange(nullptr)) {
> > 
> > I don't think we ever declare variables in if () statements in
> > libcamera, you may want to write this as
> 
> > 
> > 	GstEvent *event = self->pending_eos.exchange(nullptr);
> > 	if (event) {
> > [...]
> 
> IIRC that is valid since at least C++11. Or is this more of a stylistic choice?

It's a coding style choice. I really dislike assignments in if ()
statements, such as

	GstEvent *event;

	if (event = self->pending_eos.exchange(nullptr))

because it's very error-prone (easy to confuse the assignement and
comparison operator during review). The kernel coding style, which
influenced me a lot, doesn't like it either, and compilers generate a
warning these days.

The error-prone argument is obviously moot if declaring the variable in
the if () statement, as

	if (GstEvent *event = self->pending_eos.exchange(nullptr))

would compile and

	if (GstEvent *event == self->pending_eos.exchange(nullptr))

wouldn't (hopefully, I haven't tried). It still "feels" weird due to the
assignment-in-if-statements explanation, so maybe we can accept this in
the coding style. I'm OK doing so in the implementation of the GStreamer
element, even if we don't make it a standard practice in the libcamera
core.

-- 
Regards,

Laurent Pinchart