[libcamera-devel] [PATCH v2 10/12] libcamera: camera_manager: add CameraManager class

[Niklas Söderlund]

Hi Jacopo, Laurent,

On 2018-12-31 00:39:12 +0200, Laurent Pinchart wrote:
> Hello,
> 
> On Sunday, 30 December 2018 23:17:15 EET Jacopo Mondi wrote:
> > On Sun, Dec 30, 2018 at 09:31:09PM +0100, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > > 
> > > Thanks for your feedback.
> > 
> > [snip]
> > 
> > > On 2018-12-30 11:46:12 +0100, Jacopo Mondi wrote:
> > > > > +	/*
> > > > > +	 * TODO: Try to read handlers and order from configuration
> > > > > +	 * file and only fallback on all handlers if there is no
> > > > > +	 * configuration file.
> > > > > +	 */
> > > > > +	PipelineHandlerFactory::handlers(handlers);
> > > > 
> > > > I haven't commented on the PipelineHanderFactory patch, but the
> > > > 'handlers()' method, even if static, is a canonical getter. Passing
> > > > 'handlers' by reference make it hard to notice this actually a output
> > > > parameters. Might you consider:
> > > 
> > > It is listed as a out parameter in the documentation.
> > > 
> > > > 1) passing handlers as pointer to make it clear is a output parameter
> > > 
> > > Would this not be just as unclear as a reference? By using a pointer
> > > here all we gain is that we need to add a nullptr check in
> > > PipelineHandlerFactory::handlers.
> > 
> > I tend to agree with what is written here:
> > https://google.github.io/styleguide/cppguide.html#Output_Parameters
> > 
> > References are good as const input arguments, but for output
> > parameters it's clearer to keep them pointers, to express the
> > intent to modify their content. To me a function that returns void
> > and a (&handler) parameter clearly states it is going to modify it.
> > The first time I read handlers(handlers) I had to go back and look at
> > documentation.
> > 
> > > > 2) have the handlers() method return a vector
> > > 
> > > Sure it could be done but that would need to be a return by copy. What
> > > do rest of you think? I will leave it as is for now.
> > 
> > That would read even more natural to me:
> >         handlers = PipelineHandlerFactory::handlers();
> > 
> > But to avoid copying the vector back I would go for the argument
> > passed by pointer option.
> 
> 	std::vector<std::string> handlers = PipelineHandlerFactory::handlers();
> 
> should be optimized by the compiler to avoid copies. The std::vector<> 
> declared on the stack inside the handlers() function should be replaced with 
> the one from the caller transparently.
> 
> https://en.wikipedia.org/wiki/Copy_elision#Return_value_optimization

I yield :-)

> 
> > Or, could the PipelineHandlerFactory store a static vector, and return
> > a reference to it? I also feel is more 'natural' to return references
> > to statically allocated variables, such as member data, and return
> > pointers to instances/variables dynamically allocated. I have not
> > found that coded in any style guide, so that might just be my
> > preference only.
> > 
> > > > > +
> > 
> > [snip]
> > 
> > > > Again, minor stuff to be addressed later if you want to push this
> > > > series.
> > > 
> > > With the above addressed can i add your review tag?
> > 
> > For this and other patches where I forgot to do so:
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks Jacopo.

-- 
Regards,
Niklas Söderlund