| Message ID | 20230920151921.31273-2-andrey.konovalov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 >
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@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 > >
Hi Jacopo, Thank you for the detailed review! On 21.09.2023 10:47, Jacopo Mondi 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@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 Yes, this is much better. >> */ >> >> /** >> - * \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.. Indeed, the only place where the factory needs the "the whole" MediaDevice is when it creates an instance of converter (as 'MediaDevice *' needs to be passed to the Converter constructor). But still describing an argument of 'MediaDevice *' type as "Name of the factory" looks rather confusing to me. >> + * \param[in] media the media device to create the converter for > ^ The Right. Will fix that. >> * >> * \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 ? No, I meant "the name of which"... > I have troubles parsing this one, but I'm not a native speaker, so it > might be me ... but this indeed is hard to get, and this part should be rephrased. > 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. Yes, this sounds much more clear. >> + * 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 Thanks, Andrey >> */ >> void ConverterFactoryBase::registerType(ConverterFactoryBase *factory) >> { >> -- >> 2.34.1 >>
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 */ /** - * \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 + * \param[in] media the media device to create the converter for * * \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. + * 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. */ void ConverterFactoryBase::registerType(ConverterFactoryBase *factory) {
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> --- src/libcamera/converter.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)