| Message ID | 20220720081534.26400-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | 8b02645845bf404fba83910818b285b652f14f89 |
| Headers | show |
| Series |
|
| Related | show |
On Wed, 20 Jul 2022 at 09:15, Naushir Patuck <naush@raspberrypi.com> 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@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>(); ? > } > > template<typename T, typename V> > -- > 2.25.1 > >
Hi Naush, Thank you for the patch. On Wed, Jul 20, 2022 at 09:15:34AM +0100, Naushir Patuck via libcamera-devel 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 s/avoding/avoiding/ > 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@raspberrypi.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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>(); > } > > template<typename T, typename V>
Hi Naush, Thank you for the patch On 7/20/22 13:45, Naushir Patuck via libcamera-devel 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@raspberrypi.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.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>(); > } > > template<typename T, typename V>
Hi Naush, 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@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). > > } > > > > template<typename T, typename V>
Hi Laurent, On Wed, 20 Jul 2022 at 11:19, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > 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@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 :-) Naush > > > > } > > > > > > template<typename T, typename V> > > -- > Regards, > > 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@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>
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>(); } template<typename T, typename V>
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@raspberrypi.com> --- include/libcamera/controls.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)