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

[Laurent Pinchart]

On Tue, Jan 23, 2024 at 03:34:53PM +0200, Laurent Pinchart wrote:
> 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.

Just to be clear, as there's no .cpp file yet, I think most of the
SharedMemObject constructor should go to a separate init(size_t size)
function moved to a .cpp file, as it doesn't otherwise depend on the
type T or arguments Args.

The way we create a single memfd for every SharedMemObject isn't very
nice, as it could cause lots of fd allocations if callers are not aware
that they should create a single large object instead. This should be at
the very least clearly explained in the documentation. Ideally the
class should be refactored to make it possible to allocate multiple
objects within a single shared memory segment, but that's out of scope
for this series.

As for squashing documentation with the patch that introduces the class,
I hadn't realized that the class had simply be moved from the rpi code.
I'm OK keeping the two patches separate.

> > > + */
> > >  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);

We don't use all uppercase constants. You can name this kSize or size.

> > >  
> > >         SharedMemObject()
> > > @@ -30,6 +40,11 @@ public:
> > >         {
> > >         }
> > >  
> > > +       /**
> > > +        * \brief Contstructor for the SharedMemObject.
> > > +        * \param[in] name The requested name.
> > > +        * \param[in] args Any additional args.
> > > +        */

You need to expand the documentation. Consider the point of view of a
reader who hasn't seen the code. They can't tell from the documentation
what the name and args are for.

Also, I'd like unit tests.

> > >         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