[libcamera-devel] [PATCH] libcamera: Declare functions before variables in class definitions
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 2 13:29:23 CEST 2020
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.
> 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 */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list