[libcamera-devel] [PATCH v2 05/18] libcamera: internal: Document the SharedMemObject class

[Laurent Pinchart]

On Sat, Jan 13, 2024 at 10:45:14PM +0000, 📷-dev wrote:
> Quoting Hans de Goede via libcamera-devel (2024-01-13 14:22:05)
> > From: Dennis Bonke <admin at dennisbonke.com>
> > 
> > Document the SharedMemObject class.
> > 
> > Signed-off-by: Dennis Bonke <admin at dennisbonke.com>
> > Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> > Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
> > Tested-by: Pavel Machek <pavel at ucw.cz>
> > ---
> >  .../libcamera/internal/shared_mem_object.h    | 53 +++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> > 
> > diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h
> > index bfb639ee..e862ce48 100644
> > --- a/include/libcamera/internal/shared_mem_object.h
> > +++ b/include/libcamera/internal/shared_mem_object.h
> > @@ -19,10 +19,20 @@
> >  
> >  namespace libcamera {
> >  
> > +/**
> > + * \class SharedMemObject
> > + * \brief Helper class for shared memory allocations.
> > + *
> > + * Takes a template T which is used to indicate the
> > + * data type of the object stored.
> 
> I'd wrap this to the usual 80 chars. Not critical though.

Not critical, but no reason not to do so :-)

> It might be nice to explain here what the class is doing, as the code
> may not be visible to the reader.
> 
> The only part I'd add is perhaps something like:
> 
> """
> Memory is allocated and exposed as a SharedFD for use across IPC
> boundaries.
> """

I would like more documentation too, with an explanation of how this is
supposed to be used.

> But either way,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Documentation goes to .cpp files. The documentation should also come in
the same patch as the one that adds the class.

> > + */
> >  template<class T>
> >  class SharedMemObject
> >  {
> >  public:
> > +       /**
> > +        * \brief The size of the object that is going to be stored here.
> > +        */
> >         static constexpr std::size_t SIZE = sizeof(T);
> >  
> >         SharedMemObject()
> > @@ -30,6 +40,11 @@ public:
> >         {
> >         }
> >  
> > +       /**
> > +        * \brief Contstructor for the SharedMemObject.
> > +        * \param[in] name The requested name.
> > +        * \param[in] args Any additional args.
> > +        */
> >         template<class... Args>
> >         SharedMemObject(const std::string &name, Args &&...args)
> >                 : name_(name), obj_(nullptr)
> > @@ -57,6 +72,10 @@ public:
> >                 obj_ = new (mem) T(std::forward<Args>(args)...);
> >         }
> >  
> > +       /**
> > +        * \brief Move constructor for SharedMemObject.
> > +        * \param[in] rhs The object to move.
> > +        */
> >         SharedMemObject(SharedMemObject<T> &&rhs)
> >         {
> >                 this->name_ = std::move(rhs.name_);
> > @@ -76,6 +95,10 @@ public:
> >         /* Make SharedMemObject non-copyable for now. */
> >         LIBCAMERA_DISABLE_COPY(SharedMemObject)
> >  
> > +       /**
> > +        * \brief Operator= for SharedMemObject.
> > +        * \param[in] rhs The SharedMemObject object to take the data from.
> > +        */
> >         SharedMemObject<T> &operator=(SharedMemObject<T> &&rhs)
> >         {
> >                 this->name_ = std::move(rhs.name_);
> > @@ -85,31 +108,61 @@ public:
> >                 return *this;
> >         }
> >  
> > +       /**
> > +        * \brief Operator-> for SharedMemObject.
> > +        *
> > +        * \return the object.
> > +        */
> >         T *operator->()
> >         {
> >                 return obj_;
> >         }
> >  
> > +       /**
> > +        * \brief Operator-> for SharedMemObject.
> > +        *
> > +        * \return the object.
> > +        */
> >         const T *operator->() const
> >         {
> >                 return obj_;
> >         }
> >  
> > +       /**
> > +        * \brief Operator* for SharedMemObject.
> > +        *
> > +        * \return the object.
> > +        */
> >         T &operator*()
> >         {
> >                 return *obj_;
> >         }
> >  
> > +       /**
> > +        * \brief Operator* for SharedMemObject.
> > +        *
> > +        * \return the object.
> > +        */
> >         const T &operator*() const
> >         {
> >                 return *obj_;
> >         }
> >  
> > +       /**
> > +        * \brief Gets the file descriptor for the underlaying storage file.
> > +        *
> > +        * \return the file descriptor.
> > +        */
> >         const SharedFD &fd() const
> >         {
> >                 return fd_;
> >         }
> >  
> > +       /**
> > +        * \brief Operator bool() for SharedMemObject.
> > +        *
> > +        * \return true if the object is not null, false otherwise.
> > +        */
> >         explicit operator bool() const
> >         {
> >                 return !!obj_;

-- 
Regards,

Laurent Pinchart