[libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager

[Tomi Valkeinen]

On 18/08/2022 22:06, Laurent Pinchart wrote:
> On Thu, Aug 18, 2022 at 05:26:20PM +0200, Jacopo Mondi wrote:
>> On Thu, Aug 18, 2022 at 06:14:41PM +0300, Tomi Valkeinen wrote:
>>> On 18/08/2022 18:00, Jacopo Mondi wrote:
>>>
>>>>>>> +PyCameraManager::PyCameraManager()
>>>>>>> +{
>>>>>>> +	LOG(Python, Debug) << "PyCameraManager()";
>>>>>>
>>>>>> Don't you need to LOG_DECLARE_CATEGORY() ?
>>>>>
>>>>> It's in py_main.h.
>>>>>
>>>>
>>>> Ah, I overlooked the first patches as they were reviewed already.
>>>> Well, that header only serves for that purpose.
>>>>
>>>> I would have DEFINE_CATEGORY in py_main.cpp and DECLARE_CATEGORY()
>>>> and spare that header.
>>>
>>> Hmm, sorry can you clarify what you mean?
>>
>> If I'm not mistaken py_main.h will only contain LOG_DEFINE_CATEGORY().
>>
>> I would have DEFINE_CATEGORY() in py_main.cpp and use
>> LOG_DECLARE_CATEGORY() (which expands to an 'extern' declaration) in
>> the other compilation units that use the log symbol (which if I'm not
>> mistaken is py_camera_manager.cpp only). In this way you can get rid
>> of py_main.h completely.
> 
> Note that LOG_DECLARE_CATEGORY is similar to declaring a function
> prototype or external variable. Having it in a header file makes sense
> to me, but I would also not be against doing it manually in the .cpp
> files that need it, probably mostly because we do that already in a few
> places.

I think py_main.h is a logical place for LOG_DECLARE_CATEGORY, as the 
counterpart is in py_main.cpp. It's true that, at the moment, only 
single .cpp file uses LOG(), and there's nothing else in the py_main.h, 
but...feels like a bit needless optimization to try to get rid of the file.

  Tomi