[libcamera-devel] [PATCH v2] libcamera: controls: Suppress error message from ControlList::get()

[Laurent Pinchart]

Hi Naush,

On Thu, Jul 21, 2022 at 08:04:32AM +0100, Naushir Patuck wrote:
> On Wed, 20 Jul 2022 at 11:19, Laurent Pinchart wrote:
> > On Wed, Jul 20, 2022 at 09:17:45AM +0100, Naushir Patuck via libcamera-devel wrote:
> > > On Wed, 20 Jul 2022 at 09:15, Naushir Patuck wrote:
> > >
> > > > Now that ControlList::get() returns a std::optional<T> to handle missing
> > > > controls, the error log message in the call to ControlList::find() is
> > > > unnecessary and likely invalid.
> > > >
> > > > Fix this by avoding the call to ControlList::find() from
> > > > ControlList::get() and
> > > > replacing with a call to the underlying std::unordered_map::find().
> > > >
> > > > Fixes: 1c4d48018505 ("libcamera: controls: Use std::optional to handle
> > > > invalid control values")
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > ---
> > > >  include/libcamera/controls.h | 7 ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > > index 192be78434dc..8362fce813fb 100644
> > > > --- a/include/libcamera/controls.h
> > > > +++ b/include/libcamera/controls.h
> > > > @@ -376,11 +376,12 @@ public:
> > > >         template<typename T>
> > > >         std::optional<T> get(const Control<T> &ctrl) const
> > > >         {
> > > > -               const ControlValue *val = find(ctrl.id());
> > > > -               if (!val)
> > > > +               const auto entry = controls_.find(ctrl.id());
> > > > +               if (entry == controls_.end())
> > > >                         return std::nullopt;
> > > >
> > > > -               return val->get<T>();
> > > > +               const ControlValue &val = entry->second;
> > > > +               return val.get<T>();
> > >
> > > Bonus marks if somebody could explain to me why I need to cast
> > > entry->second to a const for get() to work...?  If I don't, the following
> > > statement:
> > >
> > > return entry->second.get<T>()
> > >
> > > returns the following compile error:
> > >
> > > ../include/libcamera/controls.h: In member function ‘std::optional<_Tp>
> > > libcamera::ControlList::get(const libcamera::Control<T>&) const’:
> > > ../include/libcamera/controls.h:384:33: error: expected primary-expression before ‘>’ token
> > >   384 |   return ((entry->second)).get<T>();
> > >       |                                 ^
> > > ../include/libcamera/controls.h:384:35: error: expected primary-expression before ‘)’ token
> > >   384 |   return ((entry->second)).get<T>();
> > >
> > > ?
> >
> > You need to write
> >
> >         return entry->second.template get<T>();
> >
> > Quoting the C++ standard,
> >
> > When the name of a member template specialization appears after . or ->
> > in a postfix-expression, or after nested-name-specifier in a
> > qualified-id, and the postfix-expression or qualified-id explicitly
> > depends on a template-parameter (14.6.2), the member template name must
> > be prefixed by the keyword template. Otherwise the name is assumed to
> > name a non-template.
> >
> > The "The template disambiguator for dependent names" section in
> > https://en.cppreference.com/w/cpp/language/dependent_name is easier to
> > understand for us humans (I didn't say easy though).
> 
> Wow, I would have never ever guessed that as a solution :-)

As much as I like C++ and (some aspects of) templates, this is a part of
the language that can only be considered as horrible. The decision to
shape C++ for backward compatibility with C definitely hurts sometimes.

> > > >         }
> > > >
> > > >         template<typename T, typename V>

-- 
Regards,

Laurent Pinchart