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

[Jacopo Mondi]

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
>