<div dir="ltr"><div dir="ltr">Hi all,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 10 Mar 2023 at 13:41, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Dave,<br>
<br>
On Fri, Mar 10, 2023 at 11:58:53AM +0000, Dave Jones wrote:<br>
> On Fri, Mar 10, 2023 at 12:40:33PM +0200, Laurent Pinchart wrote:<br>
> [snip]<br>
> >> -static std::map<std::string, CamHelperCreateFunc> camHelpers;<br>
> >> +namespace {<br>
> >> +<br>
> >> +std::map<std::string, CamHelperCreateFunc> &camHelpers()<br>
> >> +{<br>
> >> +  static std::unique_ptr<std::map<std::string, CamHelperCreateFunc>> obj =<br>
> >> +          std::make_unique<std::map<std::string, CamHelperCreateFunc>>();<br>
> >> +  return *obj;<br>
> ><br>
> > I think you could simply write<br>
> ><br>
> >     static std::map<std::string, CamHelperCreateFunc> helpers;<br>
> >     return helpers;<br>
> <br>
> That would likely be fine, but I thought I'd just note that in my <br>
> original patch [1] I'd actually used "new" with a pointer instead of a <br>
> static object quite deliberately. As my C++ skills are caked in the dust <br>
> of grime of numerous years of neglect, I'd consulted the C++ FAQ and <br>
> noted that using a static pointer opens the possibility of a crash on <br>
> deinit [2] (I'm not certain it *would* crash, but I figured I'd take the <br>
> easy/safe way out and leave the OS to deal with the memory) by using the <br>
> solution posited at [3].<br>
> <br>
> [1]: <a href="https://github.com/raspberrypi/libcamera/pull/46/files" rel="noreferrer" target="_blank">https://github.com/raspberrypi/libcamera/pull/46/files</a><br>
> <br>
> [2]: <a href="https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2" rel="noreferrer" target="_blank">https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2</a><br>
> <br>
> [3]: <a href="https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use" rel="noreferrer" target="_blank">https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use</a><br>
> <br>
> That *might* be preferable, though I admit it looks ... somewhat ugly.<br>
<br>
Unless I'm mistaken, given that you're using a unique pointer, the<br>
object will get deleted at deinit time. It should thus be functionally<br>
equivalent to the static variable in v2.<br>
<br>
We *could* allocate the objects dynamically without std::unique_ptr to<br>
avoid deinit issues, but we would then leak memory. That won't cause any<br>
issue in production, but for development it's nice if we can run<br>
libcamera applications under valgrind and expect no leaks.<br></blockquote><div><br></div><div>The unique_ptr was my addition to avoid leaks in debug.</div><div>I think using a static object like in v2 ought to be good for this fix.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>