[libcamera-devel] [PATCH v3 06/17] py: Use UniqueFD

[Tomi Valkeinen]

On 18/08/2022 17:51, Jacopo Mondi wrote:
> 
> On Thu, Aug 18, 2022 at 05:43:46PM +0300, Tomi Valkeinen wrote:
>> On 18/08/2022 17:23, Jacopo Mondi wrote:
>>> Hi Tomi
>>>
>>> On Fri, Jul 01, 2022 at 11:45:10AM +0300, Tomi Valkeinen wrote:
>>>> Use UniqueFD to automate the eventfd lifetime management.
>>>>
>>>
>>> Here you go :)
>>>
>>> Can you squash with the previous patch  ?
>>
>> No, I don't think it's a good idea to squash changes like this. The point of
>> the previous patch was to convert the existing code to PyCameraManager. I
>> don't do any changes there that are not needed.
>>
>> Squashing this, EFD_CLOEXEC and the mutex change would hide these changes
>> inside the large refactoring done in the previous patch.
>>
> 
> You'reintroducing a new class, and then patching it on top.

Well, I see it as encapsulating the existing code to be inside a class, 
i.e. moving code. =)

It is true that I could first do the small changes, and only after those 
changes do the refactoring. I have them in the current order due to the 
time the things were implemented.

> I understand you have moved existing code inside that class, but it's
> a new component.

Yes, but isn't it much more difficult to review if, while moving the 
code inside a class, I also change the code (when not needed)?

> Anyway, converting to use UniqueFD + EFD_CLOEXEC (or even better the
> one liner eventfd_ = UniqueFD(eventfd(...));) can be made one change.

True, but... Why? One shouldn't squash unrelated commits together. If I 
did what you suggest, then in the commit descs I would have to write 
something like "This patch does two things:..." or first describe the 
first change and then continue "also, we change...". When you write such 
things to a commit desc, it's almost a sure sign that you should split 
the patch =).

  Tomi