[libcamera-devel] [PATCH] libcamera: Declare functions before variables in class definitions
Umang Jain
email at uajain.com
Tue Jun 2 17:27:03 CEST 2020
Hi Laurent,
On 6/2/20 4:59 PM, Laurent Pinchart wrote:
> Hi Umang,
>
> On Tue, May 26, 2020 at 05:24:52AM +0000, Umang Jain wrote:
>> On Tue, 2020-05-26 at 06:26 +0300, Laurent Pinchart wrote:
>>> The preferred coding style in libcamera is to declare private functions
>>> before private variables in class definitions. This rule isn't followed
>>> by some of the internal classes. Update them accordingly.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> ---
>>> .../internal/device_enumerator_udev.h | 16 ++++++------
>>> .../internal/event_dispatcher_poll.h | 12 ++++-----
>>> include/libcamera/internal/ipa_manager.h | 4 +--
>>> include/libcamera/internal/ipa_module.h | 4 +--
>>> include/libcamera/internal/media_device.h | 26 +++++++++------
>>> ----
>>> include/libcamera/internal/media_object.h | 4 +--
>>> 6 files changed, 33 insertions(+), 33 deletions(-)
>> I spotted one more place that is not included in this patch, which
>> is : include/libcamera/ipa_context_wrapper.h
> In that file, we have
>
> private:
> static void queue_frame_action(void *ctx, unsigned int frame,
> struct ipa_operation_data &data);
> static const struct ipa_callback_ops callbacks_;
>
> void doQueueFrameAction(unsigned int frame,
> const IPAOperationData &data);
>
> struct ipa_context *ctx_;
> IPAInterface *intf_;
>
> ControlSerializer serializer_;
>
> I'm not sure if we should keep the two static members together, or
> strictly group all data after the functions. I went for the former as
> it's a grey area, but if you think the latter is better, I'll change it.
Indeed a grey area and after some thinking I agree that static members
should be kept together,
so this change can be merged as is.
Thanks.
>> Apart from that:
>> Reviewed-by: Umang Jain <email at uajain.com>
>>
>>> diff --git a/include/libcamera/internal/device_enumerator_udev.h
>>> b/include/libcamera/internal/device_enumerator_udev.h
>>> index fdaa20968ef0..10d17ed2abac 100644
>>> --- a/include/libcamera/internal/device_enumerator_udev.h
>>> +++ b/include/libcamera/internal/device_enumerator_udev.h
>>> @@ -36,10 +36,6 @@ public:
>>> int enumerate() final;
>>>
>>> private:
>>> - struct udev *udev_;
>>> - struct udev_monitor *monitor_;
>>> - EventNotifier *notifier_;
>>> -
>>> using DependencyMap = std::map<dev_t, std::list<MediaEntity
>>> *>>;
>>>
>>> struct MediaDeviceDeps {
>>> @@ -58,16 +54,20 @@ private:
>>> DependencyMap deps_;
>>> };
>>>
>>> - std::set<dev_t> orphans_;
>>> - std::list<MediaDeviceDeps> pending_;
>>> - std::map<dev_t, MediaDeviceDeps *> devMap_;
>>> -
>>> int addUdevDevice(struct udev_device *dev);
>>> int populateMediaDevice(MediaDevice *media, DependencyMap
>>> *deps);
>>> std::string lookupDeviceNode(dev_t devnum);
>>>
>>> int addV4L2Device(dev_t devnum);
>>> void udevNotify(EventNotifier *notifier);
>>> +
>>> + struct udev *udev_;
>>> + struct udev_monitor *monitor_;
>>> + EventNotifier *notifier_;
>>> +
>>> + std::set<dev_t> orphans_;
>>> + std::list<MediaDeviceDeps> pending_;
>>> + std::map<dev_t, MediaDeviceDeps *> devMap_;
>>> };
>>>
>>> } /* namespace libcamera */
>>> diff --git a/include/libcamera/internal/event_dispatcher_poll.h
>>> b/include/libcamera/internal/event_dispatcher_poll.h
>>> index 1f0738617425..3c9099660c4d 100644
>>> --- a/include/libcamera/internal/event_dispatcher_poll.h
>>> +++ b/include/libcamera/internal/event_dispatcher_poll.h
>>> @@ -41,16 +41,16 @@ private:
>>> EventNotifier *notifiers[3];
>>> };
>>>
>>> - std::map<int, EventNotifierSetPoll> notifiers_;
>>> - std::list<Timer *> timers_;
>>> - int eventfd_;
>>> -
>>> - bool processingEvents_;
>>> -
>>> int poll(std::vector<struct pollfd> *pollfds);
>>> void processInterrupt(const struct pollfd &pfd);
>>> void processNotifiers(const std::vector<struct pollfd>
>>> &pollfds);
>>> void processTimers();
>>> +
>>> + std::map<int, EventNotifierSetPoll> notifiers_;
>>> + std::list<Timer *> timers_;
>>> + int eventfd_;
>>> +
>>> + bool processingEvents_;
>>> };
>>>
>>> } /* namespace libcamera */
>>> diff --git a/include/libcamera/internal/ipa_manager.h
>>> b/include/libcamera/internal/ipa_manager.h
>>> index 2412d75746ac..16d742918cf2 100644
>>> --- a/include/libcamera/internal/ipa_manager.h
>>> +++ b/include/libcamera/internal/ipa_manager.h
>>> @@ -29,8 +29,6 @@ public:
>>> uint32_t minVersion);
>>>
>>> private:
>>> - std::vector<IPAModule *> modules_;
>>> -
>>> IPAManager();
>>> ~IPAManager();
>>>
>>> @@ -40,6 +38,8 @@ private:
>>>
>>> bool isSignatureValid(IPAModule *ipa) const;
>>>
>>> + std::vector<IPAModule *> modules_;
>>> +
>>> #if HAVE_IPA_PUBKEY
>>> static const uint8_t publicKeyData_[];
>>> static const PubKey pubKey_;
>>> diff --git a/include/libcamera/internal/ipa_module.h
>>> b/include/libcamera/internal/ipa_module.h
>>> index 5b54cb31a48a..788e31d8bf03 100644
>>> --- a/include/libcamera/internal/ipa_module.h
>>> +++ b/include/libcamera/internal/ipa_module.h
>>> @@ -42,6 +42,8 @@ protected:
>>> std::string logPrefix() const override;
>>>
>>> private:
>>> + int loadIPAModuleInfo();
>>> +
>>> struct IPAModuleInfo info_;
>>> std::vector<uint8_t> signature_;
>>>
>>> @@ -52,8 +54,6 @@ private:
>>> void *dlHandle_;
>>> typedef struct ipa_context *(*IPAIntfFactory)(void);
>>> IPAIntfFactory ipaCreate_;
>>> -
>>> - int loadIPAModuleInfo();
>>> };
>>>
>>> } /* namespace libcamera */
>>> diff --git a/include/libcamera/internal/media_device.h
>>> b/include/libcamera/internal/media_device.h
>>> index 9fe76c514b17..19af059d9291 100644
>>> --- a/include/libcamera/internal/media_device.h
>>> +++ b/include/libcamera/internal/media_device.h
>>> @@ -58,26 +58,13 @@ protected:
>>> std::string logPrefix() const;
>>>
>>> private:
>>> - std::string driver_;
>>> - std::string deviceNode_;
>>> - std::string model_;
>>> - unsigned int version_;
>>> -
>>> - int fd_;
>>> - bool valid_;
>>> - bool acquired_;
>>> - bool lockOwner_;
>>> -
>>> int open();
>>> void close();
>>>
>>> - std::map<unsigned int, MediaObject *> objects_;
>>> MediaObject *object(unsigned int id);
>>> bool addObject(MediaObject *object);
>>> void clear();
>>>
>>> - std::vector<MediaEntity *> entities_;
>>> -
>>> struct media_v2_interface *findInterface(const struct
>>> media_v2_topology &topology,
>>> unsigned int
>>> entityId);
>>> bool populateEntities(const struct media_v2_topology
>>> &topology);
>>> @@ -87,6 +74,19 @@ private:
>>>
>>> friend int MediaLink::setEnabled(bool enable);
>>> int setupLink(const MediaLink *link, unsigned int flags);
>>> +
>>> + std::string driver_;
>>> + std::string deviceNode_;
>>> + std::string model_;
>>> + unsigned int version_;
>>> +
>>> + int fd_;
>>> + bool valid_;
>>> + bool acquired_;
>>> + bool lockOwner_;
>>> +
>>> + std::map<unsigned int, MediaObject *> objects_;
>>> + std::vector<MediaEntity *> entities_;
>>> };
>>>
>>> } /* namespace libcamera */
>>> diff --git a/include/libcamera/internal/media_object.h
>>> b/include/libcamera/internal/media_object.h
>>> index 748eafdc880b..e8f2f27ce99e 100644
>>> --- a/include/libcamera/internal/media_object.h
>>> +++ b/include/libcamera/internal/media_object.h
>>> @@ -107,6 +107,8 @@ private:
>>> MediaEntity(const MediaEntity &) = delete;
>>> ~MediaEntity();
>>>
>>> + void addPad(MediaPad *pad);
>>> +
>>> std::string name_;
>>> unsigned int function_;
>>> unsigned int flags_;
>>> @@ -115,8 +117,6 @@ private:
>>> unsigned int minor_;
>>>
>>> std::vector<MediaPad *> pads_;
>>> -
>>> - void addPad(MediaPad *pad);
>>> };
>>>
>>> } /* namespace libcamera */
More information about the libcamera-devel
mailing list