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

[Laurent Pinchart]

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

> 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>

-- 
Regards,

Laurent Pinchart