| Message ID | 20210812173907.204700-1-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Thu, Aug 12, 2021 at 07:39:07PM +0200, Jacopo Mondi wrote: > The compiler generated constructor does not initialize the > ControlInfoMap::idmap_ field. > > Fix this by explicitly defining an empty constructor for the class. > > Reported-by: Coverity CID=354657 > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/controls.h | 2 +- > src/libcamera/controls.cpp | 8 ++++++++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 9b0d5a545301..7352c62ec03c 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -307,7 +307,7 @@ class ControlInfoMap : private std::unordered_map<const ControlId *, ControlInfo > public: > using Map = std::unordered_map<const ControlId *, ControlInfo>; > > - ControlInfoMap() = default; > + ControlInfoMap(); > ControlInfoMap(const ControlInfoMap &other) = default; > ControlInfoMap(std::initializer_list<Map::value_type> init, > const ControlIdMap &idmap); There's also the option of declaring const ControlIdMap *idmap_ = nullptr; I'm not sure what the pros and cons are, I'm pointing it out hoping that someone could tell me :-) Regardless of which option you pick, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 5c05ba4a7cd0..83f61fd96b24 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -636,6 +636,14 @@ std::string ControlInfo::toString() const > * \brief The base std::unsorted_map<> container > */ > > +/** > + * \brief Construct an empty ControlInfoMap > + */ > +ControlInfoMap::ControlInfoMap() > + : idmap_(nullptr) > +{ > +} > + > /** > * \fn ControlInfoMap::ControlInfoMap(const ControlInfoMap &other) > * \brief Copy constructor, construct a ControlInfoMap from a copy of \a other
On 12/08/2021 20:01, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Aug 12, 2021 at 07:39:07PM +0200, Jacopo Mondi wrote: >> The compiler generated constructor does not initialize the >> ControlInfoMap::idmap_ field. >> >> Fix this by explicitly defining an empty constructor for the class. >> >> Reported-by: Coverity CID=354657 >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >> --- >> include/libcamera/controls.h | 2 +- >> src/libcamera/controls.cpp | 8 ++++++++ >> 2 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >> index 9b0d5a545301..7352c62ec03c 100644 >> --- a/include/libcamera/controls.h >> +++ b/include/libcamera/controls.h >> @@ -307,7 +307,7 @@ class ControlInfoMap : private std::unordered_map<const ControlId *, ControlInfo >> public: >> using Map = std::unordered_map<const ControlId *, ControlInfo>; >> >> - ControlInfoMap() = default; >> + ControlInfoMap(); >> ControlInfoMap(const ControlInfoMap &other) = default; >> ControlInfoMap(std::initializer_list<Map::value_type> init, >> const ControlIdMap &idmap); > > There's also the option of declaring > > const ControlIdMap *idmap_ = nullptr; > > I'm not sure what the pros and cons are, I'm pointing it out hoping that > someone could tell me :-) > So far we've always done initialisation in constructor or initialisation lists. Mostly because we didn't realise earlier on that we could initialise in the declarations in the classes I think. I think declaring in the class inline and reducing the inialiser lists might end up being cleaner. I expect there won't be much difference to the compiler. > Regardless of which option you pick, > Likewise, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp >> index 5c05ba4a7cd0..83f61fd96b24 100644 >> --- a/src/libcamera/controls.cpp >> +++ b/src/libcamera/controls.cpp >> @@ -636,6 +636,14 @@ std::string ControlInfo::toString() const >> * \brief The base std::unsorted_map<> container >> */ >> >> +/** >> + * \brief Construct an empty ControlInfoMap >> + */ >> +ControlInfoMap::ControlInfoMap() >> + : idmap_(nullptr) >> +{ >> +} >> + >> /** >> * \fn ControlInfoMap::ControlInfoMap(const ControlInfoMap &other) >> * \brief Copy constructor, construct a ControlInfoMap from a copy of \a other >
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index 9b0d5a545301..7352c62ec03c 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -307,7 +307,7 @@ class ControlInfoMap : private std::unordered_map<const ControlId *, ControlInfo public: using Map = std::unordered_map<const ControlId *, ControlInfo>; - ControlInfoMap() = default; + ControlInfoMap(); ControlInfoMap(const ControlInfoMap &other) = default; ControlInfoMap(std::initializer_list<Map::value_type> init, const ControlIdMap &idmap); diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 5c05ba4a7cd0..83f61fd96b24 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -636,6 +636,14 @@ std::string ControlInfo::toString() const * \brief The base std::unsorted_map<> container */ +/** + * \brief Construct an empty ControlInfoMap + */ +ControlInfoMap::ControlInfoMap() + : idmap_(nullptr) +{ +} + /** * \fn ControlInfoMap::ControlInfoMap(const ControlInfoMap &other) * \brief Copy constructor, construct a ControlInfoMap from a copy of \a other
The compiler generated constructor does not initialize the ControlInfoMap::idmap_ field. Fix this by explicitly defining an empty constructor for the class. Reported-by: Coverity CID=354657 Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/controls.h | 2 +- src/libcamera/controls.cpp | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-)