[libcamera-devel] [PATCH v2 1/4] libcamera: converter: a few fixes to ConverterFactoryBase documentation

[Jacopo Mondi]

Hello again, sorry missed it, you also need a (possibly short) commit
message

Thanks
  j

On Thu, Sep 21, 2023 at 09:47:23AM +0200, Jacopo Mondi via libcamera-devel wrote:
> Hi Andrey
>
> On Wed, Sep 20, 2023 at 06:19:18PM +0300, Andrey Konovalov via libcamera-devel wrote:
> > Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> > ---
> >  src/libcamera/converter.cpp | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> > index fa0f1ec8..466f8421 100644
> > --- a/src/libcamera/converter.cpp
> > +++ b/src/libcamera/converter.cpp
> > @@ -199,16 +199,18 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali
> >
> >  /**
> >   * \fn ConverterFactoryBase::compatibles()
> > - * \return The names compatibles
> > + * \return The compatibles
>
> While I agree the "name compatibles" might not be super precise, just
> "the compatibles" doesn't sound any better to me.
>
> The 'compatibles' are passed in as constructor parameters and are
> documented as
>
>  * \param[in] compatibles Name aliases of the converter class
>
> I would re-use part of this in
>
> \return The list of compatible name aliases of the converter
>
> >   */
> >
> >  /**
> > - * \brief Create an instance of the converter corresponding to a named factory
> > - * \param[in] media Name of the factory
> > + * \brief Create an instance of the converter corresponding to the media device
>
> The only thing 'media' is used for is to match on its 'driver()' name,
> so the previous documentation wasn't -that- off..
>
> > + * \param[in] media the media device to create the converter for
>                        ^ The
> >   *
> >   * \return A unique pointer to a new instance of the converter subclass
> > - * corresponding to the named factory or one of its alias. Otherwise a null
> > - * pointer if no such factory exists
> > + * corresponding to the media device. The converter is created by the factory
> > + * the name or the alias of which equals to the media device driver name.
>
> s/the name/name ?
>
> I have troubles parsing this one, but I'm not a native speaker, so it
> might be me
>
> Did you mean:
>
>   * \return A unique pointer to a new instance of the converter
>   * subclass corresponding to the media device. The converter is
>   * created by matching the factory name or any of its compatible
>   * aliases with the media device driver name.
>
> > + * If the media device driver name doesn't match anything a null pointer is
> > + * returned.
> >   */
> >  std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)
> >  {
> > @@ -236,10 +238,11 @@ std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)
> >  }
> >
> >  /**
> > - * \brief Add a converter class to the registry
> > + * \brief Add a converter factory to the registry
> >   * \param[in] factory Factory to use to construct the converter class
> >   *
> > - * The caller is responsible to guarantee the uniqueness of the converter name.
> > + * The caller is responsible to guarantee the uniqueness of the converter
> > + * factory name.
>
> These are ok
>
> >   */
> >  void ConverterFactoryBase::registerType(ConverterFactoryBase *factory)
> >  {
> > --
> > 2.34.1
> >