| Message ID | 20210628200413.131701-1-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jean-Michel, On Mon, Jun 28, 2021 at 10:04:13PM +0200, Jean-Michel Hautbois wrote: > The comment is a implementation detail and does not belong to API > documentation. Move it inside the function. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/libcamera/pipeline_handler.cpp | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index f626eddd..39eed678 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -707,14 +707,14 @@ void PipelineHandlerFactory::registerType(PipelineHandlerFactory *factory) > > /** > * \brief Retrieve the list of all pipeline handler factories > - * > - * The static factories map is defined inside the function to ensures it gets > - * initialized on first use, without any dependency on link order. > - * > * \return the list of pipeline handler factories > */ > std::vector<PipelineHandlerFactory *> &PipelineHandlerFactory::factories() > { > + /* The static factories map is defined inside the function to ensures s/ensures/ensure/ (Not sure if you want to change it on a simple move patch, though) Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > + * it gets initialized on first use, without any dependency on > + * link order. > + */ > static std::vector<PipelineHandlerFactory *> factories; > return factories; > } > -- > 2.30.2 >
Hi JM, On 28/06/2021 21:04, Jean-Michel Hautbois wrote: > The comment is a implementation detail and does not belong to API > documentation. Move it inside the function. > Sounds reasonable indeed. > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/libcamera/pipeline_handler.cpp | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index f626eddd..39eed678 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -707,14 +707,14 @@ void PipelineHandlerFactory::registerType(PipelineHandlerFactory *factory) > > /** > * \brief Retrieve the list of all pipeline handler factories > - * > - * The static factories map is defined inside the function to ensures it gets > - * initialized on first use, without any dependency on link order. > - * > * \return the list of pipeline handler factories > */ > std::vector<PipelineHandlerFactory *> &PipelineHandlerFactory::factories() > { > + /* The static factories map is defined inside the function to ensures Though a multiline comment should have /* on it's own /* * line 1. * line 2. */ I'd say the s/ensures/ensure/ could be fixed up in this patch too, with just a comment in the commit message to say, it also fixes a grammatical error in the comment. With those: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + * it gets initialized on first use, without any dependency on > + * link order. > + */ > static std::vector<PipelineHandlerFactory *> factories; > return factories; > } >
On Tue, Jun 29, 2021 at 01:22:41PM +0100, Kieran Bingham wrote: > Hi JM, > > On 28/06/2021 21:04, Jean-Michel Hautbois wrote: > > The comment is a implementation detail and does not belong to API > > documentation. Move it inside the function. > > > > Sounds reasonable indeed. > > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > --- > > src/libcamera/pipeline_handler.cpp | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index f626eddd..39eed678 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -707,14 +707,14 @@ void PipelineHandlerFactory::registerType(PipelineHandlerFactory *factory) > > > > /** > > * \brief Retrieve the list of all pipeline handler factories > > - * > > - * The static factories map is defined inside the function to ensures it gets > > - * initialized on first use, without any dependency on link order. > > - * > > * \return the list of pipeline handler factories > > */ > > std::vector<PipelineHandlerFactory *> &PipelineHandlerFactory::factories() > > { > > + /* The static factories map is defined inside the function to ensures > > Though a multiline comment should have /* on it's own > > /* > * line 1. > * line 2. > */ > > I'd say the s/ensures/ensure/ could be fixed up in this patch too, with > just a comment in the commit message to say, it also fixes a grammatical > error in the comment. And the subject line should read libcamera: pipeline_handler: Hide implementation detail comment from doxygen or something similar. > With those: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + * it gets initialized on first use, without any dependency on > > + * link order. > > + */ > > static std::vector<PipelineHandlerFactory *> factories; > > return factories; > > }
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index f626eddd..39eed678 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -707,14 +707,14 @@ void PipelineHandlerFactory::registerType(PipelineHandlerFactory *factory) /** * \brief Retrieve the list of all pipeline handler factories - * - * The static factories map is defined inside the function to ensures it gets - * initialized on first use, without any dependency on link order. - * * \return the list of pipeline handler factories */ std::vector<PipelineHandlerFactory *> &PipelineHandlerFactory::factories() { + /* The static factories map is defined inside the function to ensures + * it gets initialized on first use, without any dependency on + * link order. + */ static std::vector<PipelineHandlerFactory *> factories; return factories; }
The comment is a implementation detail and does not belong to API documentation. Move it inside the function. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/libcamera/pipeline_handler.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)