| Message ID | 20191108205409.18845-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2019-11-08 22:53:48 +0200, Laurent Pinchart wrote: > The ControlList count() and find() methods use at() to lookup the > control numerical ID in the idmap_. This causes an exception to be > thrown if the ID doesn't exist in the map. Fix it by using the find() > method instead. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/controls.cpp | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 93ad2fc6a276..0c7cd449ad64 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -491,7 +491,11 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const > */ > ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > { > - return count(idmap_.at(id)); > + auto iter = idmap_.find(id); > + if (iter == idmap_.end()) > + return 0; > + > + return count(iter->second); > } > > /** > @@ -502,7 +506,11 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > */ > ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > { > - return find(idmap_.at(id)); > + auto iter = idmap_.find(id); > + if (iter == idmap_.end()) > + return end(); > + > + return find(iter->second); > } > > /** > @@ -513,7 +521,11 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > */ > ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const > { > - return find(idmap_.at(id)); > + auto iter = idmap_.find(id); > + if (iter == idmap_.end()) > + return end(); > + > + return find(iter->second); > } > > /** > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On Fri, Nov 08, 2019 at 10:53:48PM +0200, Laurent Pinchart wrote: > The ControlList count() and find() methods use at() to lookup the > control numerical ID in the idmap_. This causes an exception to be > thrown if the ID doesn't exist in the map. Fix it by using the find() > method instead. Be aware that ControlList::at() (const) still uses at() and could potentially raise an exception as well, while I think we clearly prescribes that libcamera does not uses them. If that's fine please have mine Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/controls.cpp | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 93ad2fc6a276..0c7cd449ad64 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -491,7 +491,11 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const > */ > ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > { > - return count(idmap_.at(id)); > + auto iter = idmap_.find(id); > + if (iter == idmap_.end()) > + return 0; > + > + return count(iter->second); > } > > /** > @@ -502,7 +506,11 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > */ > ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > { > - return find(idmap_.at(id)); > + auto iter = idmap_.find(id); > + if (iter == idmap_.end()) > + return end(); > + > + return find(iter->second); > } > > /** > @@ -513,7 +521,11 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > */ > ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const > { > - return find(idmap_.at(id)); > + auto iter = idmap_.find(id); > + if (iter == idmap_.end()) > + return end(); > + > + return find(iter->second); > } > > /** > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Fri, Nov 15, 2019 at 05:00:45PM +0100, Jacopo Mondi wrote: > On Fri, Nov 08, 2019 at 10:53:48PM +0200, Laurent Pinchart wrote: > > The ControlList count() and find() methods use at() to lookup the > > control numerical ID in the idmap_. This causes an exception to be > > thrown if the ID doesn't exist in the map. Fix it by using the find() > > method instead. > > Be aware that ControlList::at() (const) still uses at() and could > potentially raise an exception as well, while I think we clearly > prescribes that libcamera does not uses them. There's not much we can do about that, as at() returns a reference, so there's no way to return an error. It's an error in the caller to use at() without checking if the item exists first. We could specify this in the documentation, or maybe remove the at() method completely, forcing the caller to use find(). What do you think is best ? > If that's fine please have mine > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/controls.cpp | 18 +++++++++++++++--- > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > index 93ad2fc6a276..0c7cd449ad64 100644 > > --- a/src/libcamera/controls.cpp > > +++ b/src/libcamera/controls.cpp > > @@ -491,7 +491,11 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const > > */ > > ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > > { > > - return count(idmap_.at(id)); > > + auto iter = idmap_.find(id); > > + if (iter == idmap_.end()) > > + return 0; > > + > > + return count(iter->second); > > } > > > > /** > > @@ -502,7 +506,11 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > > */ > > ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > > { > > - return find(idmap_.at(id)); > > + auto iter = idmap_.find(id); > > + if (iter == idmap_.end()) > > + return end(); > > + > > + return find(iter->second); > > } > > > > /** > > @@ -513,7 +521,11 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > > */ > > ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const > > { > > - return find(idmap_.at(id)); > > + auto iter = idmap_.find(id); > > + if (iter == idmap_.end()) > > + return end(); > > + > > + return find(iter->second); > > } > > > > /**
Hi Laurent, On Mon, Nov 18, 2019 at 02:44:54AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Fri, Nov 15, 2019 at 05:00:45PM +0100, Jacopo Mondi wrote: > > On Fri, Nov 08, 2019 at 10:53:48PM +0200, Laurent Pinchart wrote: > > > The ControlList count() and find() methods use at() to lookup the s/ControlList/ControlInfoMap/ > > > control numerical ID in the idmap_. This causes an exception to be > > > thrown if the ID doesn't exist in the map. Fix it by using the find() > > > method instead. > > > > Be aware that ControlList::at() (const) still uses at() and could > > potentially raise an exception as well, while I think we clearly > > prescribes that libcamera does not uses them. > > There's not much we can do about that, as at() returns a reference, so > there's no way to return an error. It's an error in the caller to use > at() without checking if the item exists first. We could specify this in > the documentation, or maybe remove the at() method completely, forcing > the caller to use find(). What do you think is best ? > I don't see users of ControlInfoMap::at() if not the the ControlInfoMap class itself. I would be fine in dropping it. There are usaged of at() on maps in many places on std::map instances though, so you don't want to remove it for consitency, that's fine as well. > > If that's fine please have mine > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/libcamera/controls.cpp | 18 +++++++++++++++--- > > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > index 93ad2fc6a276..0c7cd449ad64 100644 > > > --- a/src/libcamera/controls.cpp > > > +++ b/src/libcamera/controls.cpp > > > @@ -491,7 +491,11 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const > > > */ > > > ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > > > { > > > - return count(idmap_.at(id)); > > > + auto iter = idmap_.find(id); > > > + if (iter == idmap_.end()) > > > + return 0; > > > + > > > + return count(iter->second); > > > } > > > > > > /** > > > @@ -502,7 +506,11 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > > > */ > > > ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > > > { > > > - return find(idmap_.at(id)); > > > + auto iter = idmap_.find(id); > > > + if (iter == idmap_.end()) > > > + return end(); > > > + > > > + return find(iter->second); > > > } > > > > > > /** > > > @@ -513,7 +521,11 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > > > */ > > > ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const > > > { > > > - return find(idmap_.at(id)); > > > + auto iter = idmap_.find(id); > > > + if (iter == idmap_.end()) > > > + return end(); > > > + > > > + return find(iter->second); > > > } > > > > > > /** > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Nov 18, 2019 at 11:51:21PM +0100, Jacopo Mondi wrote: > On Mon, Nov 18, 2019 at 02:44:54AM +0200, Laurent Pinchart wrote: > > On Fri, Nov 15, 2019 at 05:00:45PM +0100, Jacopo Mondi wrote: > > > On Fri, Nov 08, 2019 at 10:53:48PM +0200, Laurent Pinchart wrote: > > > > The ControlList count() and find() methods use at() to lookup the > > s/ControlList/ControlInfoMap/ > > > > > control numerical ID in the idmap_. This causes an exception to be > > > > thrown if the ID doesn't exist in the map. Fix it by using the find() > > > > method instead. > > > > > > Be aware that ControlList::at() (const) still uses at() and could > > > potentially raise an exception as well, while I think we clearly > > > prescribes that libcamera does not uses them. > > > > There's not much we can do about that, as at() returns a reference, so > > there's no way to return an error. It's an error in the caller to use > > at() without checking if the item exists first. We could specify this in > > the documentation, or maybe remove the at() method completely, forcing > > the caller to use find(). What do you think is best ? > > I don't see users of ControlInfoMap::at() if not the the > ControlInfoMap class itself. I would be fine in dropping it. There are > usaged of at() on maps in many places on std::map instances though, so > you don't want to remove it for consitency, that's fine as well. I think there's at least one user in the tests, as compilation failed when I dropped the method. Would you like to submit a patch, or do you think this is overkill ? > > > If that's fine please have mine > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > src/libcamera/controls.cpp | 18 +++++++++++++++--- > > > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > > index 93ad2fc6a276..0c7cd449ad64 100644 > > > > --- a/src/libcamera/controls.cpp > > > > +++ b/src/libcamera/controls.cpp > > > > @@ -491,7 +491,11 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const > > > > */ > > > > ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > > > > { > > > > - return count(idmap_.at(id)); > > > > + auto iter = idmap_.find(id); > > > > + if (iter == idmap_.end()) > > > > + return 0; > > > > + > > > > + return count(iter->second); > > > > } > > > > > > > > /** > > > > @@ -502,7 +506,11 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > > > > */ > > > > ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > > > > { > > > > - return find(idmap_.at(id)); > > > > + auto iter = idmap_.find(id); > > > > + if (iter == idmap_.end()) > > > > + return end(); > > > > + > > > > + return find(iter->second); > > > > } > > > > > > > > /** > > > > @@ -513,7 +521,11 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > > > > */ > > > > ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const > > > > { > > > > - return find(idmap_.at(id)); > > > > + auto iter = idmap_.find(id); > > > > + if (iter == idmap_.end()) > > > > + return end(); > > > > + > > > > + return find(iter->second); > > > > } > > > > > > > > /**
Hi Laurent, On 08/11/2019 20:53, Laurent Pinchart wrote: > The ControlList count() and find() methods use at() to lookup the > control numerical ID in the idmap_. This causes an exception to be > thrown if the ID doesn't exist in the map. Fix it by using the find() > method instead. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/controls.cpp | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 93ad2fc6a276..0c7cd449ad64 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -491,7 +491,11 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const > */ > ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > { > - return count(idmap_.at(id)); > + auto iter = idmap_.find(id); > + if (iter == idmap_.end()) > + return 0; > + > + return count(iter->second); Is this further count() redundant? Has the desired item already been 'found' at that point, and thus we can return '1'? I fear use of std::pair once again makes this hunk fairly horrible to read as a standalone code segment. Even more so with the user of auto iter. (yes, ->second is my ->first pet-peeve) Actually - Looking below, I suspect the call to count is important, because we might be counting something else. Can we add a brief comment here or something to make all of this clear? Or expand the iter->second to a defined type so it's clear /what/ we're counting. -- Kieran > } > > /** > @@ -502,7 +506,11 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > */ > ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > { > - return find(idmap_.at(id)); > + auto iter = idmap_.find(id); > + if (iter == idmap_.end()) > + return end(); > + > + return find(iter->second); > } > > /** > @@ -513,7 +521,11 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > */ > ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const > { > - return find(idmap_.at(id)); > + auto iter = idmap_.find(id); > + if (iter == idmap_.end()) > + return end(); > + > + return find(iter->second); > } > > /** >
Hi Kieran, On Tue, Nov 19, 2019 at 03:46:12PM +0000, Kieran Bingham wrote: > On 08/11/2019 20:53, Laurent Pinchart wrote: > > The ControlList count() and find() methods use at() to lookup the > > control numerical ID in the idmap_. This causes an exception to be > > thrown if the ID doesn't exist in the map. Fix it by using the find() > > method instead. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/controls.cpp | 18 +++++++++++++++--- > > 1 file changed, 15 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > index 93ad2fc6a276..0c7cd449ad64 100644 > > --- a/src/libcamera/controls.cpp > > +++ b/src/libcamera/controls.cpp > > @@ -491,7 +491,11 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const > > */ > > ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > > { > > - return count(idmap_.at(id)); > > + auto iter = idmap_.find(id); > > + if (iter == idmap_.end()) > > + return 0; > > + > > + return count(iter->second); > > Is this further count() redundant? > > Has the desired item already been 'found' at that point, and thus we can > return '1'? > > I fear use of std::pair once again makes this hunk fairly horrible to > read as a standalone code segment. Even more so with the user of auto > iter. (yes, ->second is my ->first pet-peeve) :-) I dislike it as well, but in this case it's nothing we can control. idmap_ is an std::unordered_map, and std::unordered_map::value_type is a std::pair. That's the C++ STL API. What we can do is use local variables to make this more explicit. > Actually - Looking below, I suspect the call to count is important, > because we might be counting something else. Actually I don't think so. The idmap is generated with one entry per entry in ControlInfoMap, with a 1:1 mapping between the two, so I think we can just return 1 here. Or even better, return idmap_.count(id); > Can we add a brief comment here or something to make all of this clear? > > Or expand the iter->second to a defined type so it's clear /what/ we're > counting. > > > } > > > > /** > > @@ -502,7 +506,11 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > > */ > > ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > > { > > - return find(idmap_.at(id)); > > + auto iter = idmap_.find(id); > > + if (iter == idmap_.end()) > > + return end(); > > + > > + return find(iter->second); > > } > > > > /** > > @@ -513,7 +521,11 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > > */ > > ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const > > { > > - return find(idmap_.at(id)); > > + auto iter = idmap_.find(id); > > + if (iter == idmap_.end()) > > + return end(); > > + > > + return find(iter->second); > > } > > > > /**
Hi Laurent, On 20/11/2019 03:48, Laurent Pinchart wrote: > Hi Kieran, > > On Tue, Nov 19, 2019 at 03:46:12PM +0000, Kieran Bingham wrote: >> On 08/11/2019 20:53, Laurent Pinchart wrote: >>> The ControlList count() and find() methods use at() to lookup the >>> control numerical ID in the idmap_. This causes an exception to be >>> thrown if the ID doesn't exist in the map. Fix it by using the find() >>> method instead. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> src/libcamera/controls.cpp | 18 +++++++++++++++--- >>> 1 file changed, 15 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp >>> index 93ad2fc6a276..0c7cd449ad64 100644 >>> --- a/src/libcamera/controls.cpp >>> +++ b/src/libcamera/controls.cpp >>> @@ -491,7 +491,11 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const >>> */ >>> ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const >>> { >>> - return count(idmap_.at(id)); >>> + auto iter = idmap_.find(id); >>> + if (iter == idmap_.end()) >>> + return 0; >>> + >>> + return count(iter->second); >> >> Is this further count() redundant? >> >> Has the desired item already been 'found' at that point, and thus we can >> return '1'? >> >> I fear use of std::pair once again makes this hunk fairly horrible to >> read as a standalone code segment. Even more so with the user of auto >> iter. (yes, ->second is my ->first pet-peeve) > > :-) I dislike it as well, but in this case it's nothing we can control. > idmap_ is an std::unordered_map, and std::unordered_map::value_type is a > std::pair. That's the C++ STL API. What we can do is use local variables > to make this more explicit. Yes, the use of local variables to make the naming explicit is what I meant here ... I feel like any use of ->first and ->second is the equivalent of naming variables int integer = 0; Just like the std::pair, here there is an integer, but it doesn't tell me what it stores it for, The ->first ->second /are/ the first and second parameter of the pair - but that doesn't tell me what they are used for. And it leaves the reader blind (or worse, confused). I feel strongly enough about this to say we should propose a coding standard addition to say all use of std::pair should be named explicitly by mapping to typed (and well named) local variables (which I expect the compiler to optimise out), with perhaps an exception if the std::pair return types are already exposed within the local function and "might" be redundant. What do you think ? >> Actually - Looking below, I suspect the call to count is important, >> because we might be counting something else. > > Actually I don't think so. The idmap is generated with one entry per > entry in ControlInfoMap, with a 1:1 mapping between the two, so I think > we can just return 1 here. Or even better, > > return idmap_.count(id); That sounds even better. Perhaps something like this then (Adjust as appropriate of course): ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const { /* * The idmap is generated with one entry per entry in ControlInfoMap * with a 1:1 mapping between the two, therefore we count the relevant * idmap entries to establish the ControlInfoMap count without * performing an additional lookup. */ return idmap_.count(id); } > >> Can we add a brief comment here or something to make all of this clear? >> >> Or expand the iter->second to a defined type so it's clear /what/ we're >> counting. >> >>> } >>> >>> /** >>> @@ -502,7 +506,11 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const >>> */ >>> ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) >>> { >>> - return find(idmap_.at(id)); >>> + auto iter = idmap_.find(id); >>> + if (iter == idmap_.end()) >>> + return end(); >>> + >>> + return find(iter->second); >>> } >>> >>> /** >>> @@ -513,7 +521,11 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) >>> */ >>> ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const >>> { >>> - return find(idmap_.at(id)); >>> + auto iter = idmap_.find(id); >>> + if (iter == idmap_.end()) >>> + return end(); >>> + >>> + return find(iter->second); >>> } >>> >>> /** >
Hi Kieran, On Wed, Nov 20, 2019 at 09:17:05AM +0000, Kieran Bingham wrote: > On 20/11/2019 03:48, Laurent Pinchart wrote: > > On Tue, Nov 19, 2019 at 03:46:12PM +0000, Kieran Bingham wrote: > >> On 08/11/2019 20:53, Laurent Pinchart wrote: > >>> The ControlList count() and find() methods use at() to lookup the > >>> control numerical ID in the idmap_. This causes an exception to be > >>> thrown if the ID doesn't exist in the map. Fix it by using the find() > >>> method instead. > >>> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> src/libcamera/controls.cpp | 18 +++++++++++++++--- > >>> 1 file changed, 15 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > >>> index 93ad2fc6a276..0c7cd449ad64 100644 > >>> --- a/src/libcamera/controls.cpp > >>> +++ b/src/libcamera/controls.cpp > >>> @@ -491,7 +491,11 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const > >>> */ > >>> ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > >>> { > >>> - return count(idmap_.at(id)); > >>> + auto iter = idmap_.find(id); > >>> + if (iter == idmap_.end()) > >>> + return 0; > >>> + > >>> + return count(iter->second); > >> > >> Is this further count() redundant? > >> > >> Has the desired item already been 'found' at that point, and thus we can > >> return '1'? > >> > >> I fear use of std::pair once again makes this hunk fairly horrible to > >> read as a standalone code segment. Even more so with the user of auto > >> iter. (yes, ->second is my ->first pet-peeve) > > > > :-) I dislike it as well, but in this case it's nothing we can control. > > idmap_ is an std::unordered_map, and std::unordered_map::value_type is a > > std::pair. That's the C++ STL API. What we can do is use local variables > > to make this more explicit. > > Yes, the use of local variables to make the naming explicit is what I > meant here ... > > I feel like any use of ->first and ->second is the equivalent of naming > variables > int integer = 0; > > Just like the std::pair, here there is an integer, but it doesn't tell > me what it stores it for, The ->first ->second /are/ the first and > second parameter of the pair - but that doesn't tell me what they are > used for. And it leaves the reader blind (or worse, confused). > > I feel strongly enough about this to say we should propose a coding > standard addition to say all use of std::pair should be named explicitly > by mapping to typed (and well named) local variables (which I expect the > compiler to optimise out), with perhaps an exception if the std::pair > return types are already exposed within the local function and "might" > be redundant. > > What do you think ? I agree with you, and we're already doing this in most cases. I would however relax the rule a little bit, in some cases using .first and .second is explicit enough, but that's far from being the general rule. One case where .first and .second is fine in my opinion is when they really denote the first and second instances of something, with an explicit std::pair<Type, Type>. /* French fries have to be fried twice. */ std::pair<std::chrono::duration, std::chrono::duration> cookingDuration; fry(fries, cookingTime.first); std::thread::wait_for(restDuration); fry(fries, cookingTime.second); Another case could be when the map type is explicitly shown in the code iterating on the map. I find the following explicit enough: std::map<int, std::string> theMap = generateTheMap(); for (const auto &pair : theMap) std::cout << "mapping from " << pair.first << " to " << pair.second << std::endl; But this wouldn't be explicit enough: for (const auto &pair : theMap_) std::cout << "mapping from " << pair.first << " to " << pair.second << std::endl; as the definition of theMap_ is located in a different place. > >> Actually - Looking below, I suspect the call to count is important, > >> because we might be counting something else. > > > > Actually I don't think so. The idmap is generated with one entry per > > entry in ControlInfoMap, with a 1:1 mapping between the two, so I think > > we can just return 1 here. Or even better, > > > > return idmap_.count(id); > > That sounds even better. > Perhaps something like this then (Adjust as appropriate of course): > > ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > { > /* > * The idmap is generated with one entry per entry in ControlInfoMap > * with a 1:1 mapping between the two, therefore we count the relevant > * idmap entries to establish the ControlInfoMap count without > * performing an additional lookup. > */ > > return idmap_.count(id); > } I've applied a slightly modified version. Thank you for the proposal. > >> Can we add a brief comment here or something to make all of this clear? > >> > >> Or expand the iter->second to a defined type so it's clear /what/ we're > >> counting. > >> > >>> } > >>> > >>> /** > >>> @@ -502,7 +506,11 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const > >>> */ > >>> ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > >>> { > >>> - return find(idmap_.at(id)); > >>> + auto iter = idmap_.find(id); > >>> + if (iter == idmap_.end()) > >>> + return end(); > >>> + > >>> + return find(iter->second); > >>> } > >>> > >>> /** > >>> @@ -513,7 +521,11 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) > >>> */ > >>> ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const > >>> { > >>> - return find(idmap_.at(id)); > >>> + auto iter = idmap_.find(id); > >>> + if (iter == idmap_.end()) > >>> + return end(); > >>> + > >>> + return find(iter->second); > >>> } > >>> > >>> /**
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 93ad2fc6a276..0c7cd449ad64 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -491,7 +491,11 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const */ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const { - return count(idmap_.at(id)); + auto iter = idmap_.find(id); + if (iter == idmap_.end()) + return 0; + + return count(iter->second); } /** @@ -502,7 +506,11 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const */ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) { - return find(idmap_.at(id)); + auto iter = idmap_.find(id); + if (iter == idmap_.end()) + return end(); + + return find(iter->second); } /** @@ -513,7 +521,11 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id) */ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const { - return find(idmap_.at(id)); + auto iter = idmap_.find(id); + if (iter == idmap_.end()) + return end(); + + return find(iter->second); } /**
The ControlList count() and find() methods use at() to lookup the control numerical ID in the idmap_. This causes an exception to be thrown if the ID doesn't exist in the map. Fix it by using the find() method instead. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/controls.cpp | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)