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

[Jacopo Mondi]

Hi Niklas,

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.

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
   j
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20181230/51bd5f2b/attachment.sig>