| Message ID | 20220401000616.12976-4-Rauch.Christian@gmx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Christian On Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote: > The new default constructor allows to construct a fixed-sized Span via the > default constructor of its stored data type. > This prevents the construction of empty fixed-sized Spans that cannot hold > any data. > > Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> > --- > include/libcamera/base/span.h | 5 +++++ > include/libcamera/controls.h | 2 +- > test/span.cpp | 2 +- > 3 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h > index 88d2e3de..7a4806dc 100644 > --- a/include/libcamera/base/span.h > +++ b/include/libcamera/base/span.h > @@ -112,6 +112,11 @@ public: > { > } > > + Span() > + { > + Span(std::array<value_type, extent>{}); > + } > + This constructor creates a span of 'extent' elements all of them initialized to 0 then ? If I remove it I get ../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span<const float, 2>::Span(<brace-enclosed initializer list>)’ 380 | return T{}; Caused by template<typename T> T get(const Control<T> &ctrl) const { const ControlValue *val = find(ctrl.id()); if (!val) return T{}; <------ return val->get<T>(); } as now that extent is != 0 the expression doesn't bind anymore to the existing constructor: template<bool Dependent = false, typename = std::enable_if_t<Dependent || Extent == 0>> constexpr Span() noexcept : data_(nullptr) { } (I don't fully get what is the use of Dependent in the above definition :) However I feel like creating a Span of Extent elements all zeroed is a bit hill-defined, considering it is only used to return an error condition. In example, if I'm not mistaken a Span constructed with your proposed constructor will constexpr size_type size() const noexcept { return Extent; } constexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); } constexpr bool empty() const noexcept { return size() == 0; } be !empty and of size == 2, which is misleading considering we really want to return an empty Span in case of error and the caller should not be mis-leaded thinking it is valid. I don't have any super smart proposal to offer, if not recording the 'valid' or 'empty' state in a class member and use it in size() and empty() instead of relying on Extent which is defined at overload resolution time and cannot be assigned at run time. > explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count) > : data_(ptr) > { > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 665bcac1..de8a7770 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -167,7 +167,7 @@ public: > > using V = typename T::value_type; > const V *value = reinterpret_cast<const V *>(data().data()); > - return { value, numElements_ }; > + return T{ value, numElements_ }; this applies to overload template<typename T, typename std::enable_if_t<details::is_span<T>::value || std::is_same<std::string, std::remove_cv_t<T>>::value, T get() const { assert(type_ == details::control_type<std::remove_cv_t<T>>::value); assert(isArray_); using V = typename T::value_type; const V *value = reinterpret_cast<const V *>(data().data()); return { value, numElements_ }; } Isn't the returned { value, numElements_ } already used to construct an instance of T if assigned to an lvalue expression ? IOW why do you need to contruct an instance of T and return it explicitely ? Thanks to return value optimization this shouldn't have any performance impact but I have missed why it is needed. <2 minutes later> And now that I wrote this, if I remove it I get include/libcamera/controls.h:170:46: error: converting to ‘libcamera::Span<const float, 2>’ from initializer list would use explicit constructor ‘constexpr libcamera::Span<T, Extent>::Span(libcamera::Span<T, Extent>::pointer, libcamera::Span<T, Extent>::size_type) [with T = const float; long unsigned int Extent = 2; libcamera::Span<T, Extent>::pointer = const float*; libcamera::Span<T, Extent>::size_type = long unsigned int]’ Because indeed explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count) is, well, explicit (which I think it's ok and should not be removed) I'll leave here the above text anyway for others that might have the same question :) > } > > #ifndef __DOXYGEN__ > diff --git a/test/span.cpp b/test/span.cpp > index abf3a5d6..c37e2a66 100644 > --- a/test/span.cpp > +++ b/test/span.cpp > @@ -37,7 +37,7 @@ protected: > * to generate undefined behaviour. > */ > > - Span<int, 0>{}; > + /* Span<int, 0>{}; */ You should remove it, or if the above issue with constructing an 'empty' Span of size > 0 is resolved, prove that it works as intended. Span<int, 4> s{} if (!s.empty()) error! Thanks j > /* Span<int, 4>{}; */ > > Span<int, 4>{ &i[0], 4 }; > -- > 2.25.1 >
Hi Jacopo, Thanks for reviewing this. See my responses inline below. Am 01.04.22 um 15:28 schrieb Jacopo Mondi: > Hi Christian > > On Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote: >> The new default constructor allows to construct a fixed-sized Span via the >> default constructor of its stored data type. >> This prevents the construction of empty fixed-sized Spans that cannot hold >> any data. >> >> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> >> --- >> include/libcamera/base/span.h | 5 +++++ >> include/libcamera/controls.h | 2 +- >> test/span.cpp | 2 +- >> 3 files changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h >> index 88d2e3de..7a4806dc 100644 >> --- a/include/libcamera/base/span.h >> +++ b/include/libcamera/base/span.h >> @@ -112,6 +112,11 @@ public: >> { >> } >> >> + Span() >> + { >> + Span(std::array<value_type, extent>{}); >> + } >> + > > This constructor creates a span of 'extent' elements all of them > initialized to 0 then ? All elements will be default constructed. Integers and floats will be initialised to 0, "std::strings" will be empty, and 'Size' and 'Rectangle' will use their specified default constructors. The default initialisation is required because the fixed-sized array cannot be empty. > > If I remove it I get > > ../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span<const float, 2>::Span(<brace-enclosed initializer list>)’ > 380 | return T{}; > > Caused by > template<typename T> > T get(const Control<T> &ctrl) const > { > const ControlValue *val = find(ctrl.id()); > if (!val) > return T{}; <------ > > return val->get<T>(); > } > > as now that extent is != 0 the expression doesn't bind anymore to the > existing constructor: > > template<bool Dependent = false, > typename = std::enable_if_t<Dependent || Extent == 0>> > constexpr Span() noexcept > : data_(nullptr) > { > } > > (I don't fully get what is the use of Dependent in the above definition :) I also do not understand the use of this constructor, since a 0-fixed-size Span cannot store anything. > > However I feel like creating a Span of Extent elements all zeroed is a > bit hill-defined, considering it is only used to return an error > condition. In example, if I'm not mistaken a Span constructed with > your proposed constructor will > > constexpr size_type size() const noexcept { return Extent; } > constexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); } > constexpr bool empty() const noexcept { return size() == 0; } > > be !empty and of size == 2, which is misleading considering we really > want to return an empty Span in case of error and the caller should > not be mis-leaded thinking it is valid. > > I don't have any super smart proposal to offer, if not recording the > 'valid' or 'empty' state in a class member and use it in size() and > empty() instead of relying on Extent which is defined at overload > resolution time and cannot be assigned at run time. I think this is a general issue with return default constructed objects to indicate an error. The error is only defined implicitly. If ControlList::get returns a 'Size' with width=0 and height=0, then it is not clear if this indicates an error or if this value is really stored. You could add special class members to indicate that an object is "valid", but this is not a nice solution and only works for custom types in libcamera. The "C" way would probably to indicate an error by a NULL pointer and a returned error number. I think the proper way would be to throw an exception. This could also be a custom libcamera exception. > >> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count) >> : data_(ptr) >> { >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >> index 665bcac1..de8a7770 100644 >> --- a/include/libcamera/controls.h >> +++ b/include/libcamera/controls.h >> @@ -167,7 +167,7 @@ public: >> >> using V = typename T::value_type; >> const V *value = reinterpret_cast<const V *>(data().data()); >> - return { value, numElements_ }; >> + return T{ value, numElements_ }; > > this applies to overload > > template<typename T, typename std::enable_if_t<details::is_span<T>::value || > std::is_same<std::string, std::remove_cv_t<T>>::value, > T get() const > { > assert(type_ == details::control_type<std::remove_cv_t<T>>::value); > assert(isArray_); > > using V = typename T::value_type; > const V *value = reinterpret_cast<const V *>(data().data()); > return { value, numElements_ }; > } > > Isn't the returned { value, numElements_ } already used to construct an > instance of T if assigned to an lvalue expression ? > > IOW why do you need to contruct an instance of T and return it > explicitely ? Thanks to return value optimization this shouldn't have > any performance impact but I have missed why it is needed. > > <2 minutes later> > > And now that I wrote this, if I remove it I get > > include/libcamera/controls.h:170:46: error: converting to > ‘libcamera::Span<const float, 2>’ from initializer list would use > explicit constructor ‘constexpr libcamera::Span<T, > Extent>::Span(libcamera::Span<T, Extent>::pointer, libcamera::Span<T, > Extent>::size_type) [with T = const float; long unsigned int Extent = > 2; libcamera::Span<T, Extent>::pointer = const float*; > libcamera::Span<T, Extent>::size_type = long unsigned int]’ > > Because indeed > explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count) > > is, well, explicit (which I think it's ok and should not be removed) > > I'll leave here the above text anyway for others that might have the > same question :) > >> } >> >> #ifndef __DOXYGEN__ >> diff --git a/test/span.cpp b/test/span.cpp >> index abf3a5d6..c37e2a66 100644 >> --- a/test/span.cpp >> +++ b/test/span.cpp >> @@ -37,7 +37,7 @@ protected: >> * to generate undefined behaviour. >> */ >> >> - Span<int, 0>{}; >> + /* Span<int, 0>{}; */ > > You should remove it, or if the above issue with constructing an > 'empty' Span of size > 0 is resolved, prove that it works as intended. I commented it out instead of removing this, because the comment above says "[...] Commented-out tests are expected not to compile [...]" and there was already a commented case for an 4-sized Span. Anyway, I don't see a real application of a 0-fixed-sized Span, as it will never be able to store anything. Best, Christian > > Span<int, 4> s{} > if (!s.empty()) > error! > > Thanks > j > >> /* Span<int, 4>{}; */ >> >> Span<int, 4>{ &i[0], 4 }; >> -- >> 2.25.1 >>
Hi Christian, On Sat, Apr 02, 2022 at 12:05:43AM +0100, Christian Rauch wrote: > Hi Jacopo, > > Thanks for reviewing this. See my responses inline below. > > > Am 01.04.22 um 15:28 schrieb Jacopo Mondi: > > Hi Christian > > > > On Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote: > >> The new default constructor allows to construct a fixed-sized Span via the > >> default constructor of its stored data type. > >> This prevents the construction of empty fixed-sized Spans that cannot hold > >> any data. > >> > >> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> > >> --- > >> include/libcamera/base/span.h | 5 +++++ > >> include/libcamera/controls.h | 2 +- > >> test/span.cpp | 2 +- > >> 3 files changed, 7 insertions(+), 2 deletions(-) > >> > >> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h > >> index 88d2e3de..7a4806dc 100644 > >> --- a/include/libcamera/base/span.h > >> +++ b/include/libcamera/base/span.h > >> @@ -112,6 +112,11 @@ public: > >> { > >> } > >> > >> + Span() > >> + { > >> + Span(std::array<value_type, extent>{}); > >> + } > >> + > > > > This constructor creates a span of 'extent' elements all of them > > initialized to 0 then ? > > All elements will be default constructed. Integers and floats will be > initialised to 0, "std::strings" will be empty, and 'Size' and > 'Rectangle' will use their specified default constructors. > The default initialisation is required because the fixed-sized array > cannot be empty. > You're right, "initialized to 0" is not correct, "default constructed" is the term I should have used :) > > > > If I remove it I get > > > > ../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span<const float, 2>::Span(<brace-enclosed initializer list>)’ > > 380 | return T{}; > > > > Caused by > > template<typename T> > > T get(const Control<T> &ctrl) const > > { > > const ControlValue *val = find(ctrl.id()); > > if (!val) > > return T{}; <------ > > > > return val->get<T>(); > > } > > > > as now that extent is != 0 the expression doesn't bind anymore to the > > existing constructor: > > > > template<bool Dependent = false, > > typename = std::enable_if_t<Dependent || Extent == 0>> > > constexpr Span() noexcept > > : data_(nullptr) > > { > > } > > > > (I don't fully get what is the use of Dependent in the above definition :) > > I also do not understand the use of this constructor, since a > 0-fixed-size Span cannot store anything. I think it was used to identify a non-valid span, to be returned in case of errors > > > > However I feel like creating a Span of Extent elements all zeroed is a > > bit hill-defined, considering it is only used to return an error > > condition. In example, if I'm not mistaken a Span constructed with > > your proposed constructor will > > > > constexpr size_type size() const noexcept { return Extent; } > > constexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); } > > constexpr bool empty() const noexcept { return size() == 0; } > > > > be !empty and of size == 2, which is misleading considering we really > > want to return an empty Span in case of error and the caller should > > not be mis-leaded thinking it is valid. > > > > I don't have any super smart proposal to offer, if not recording the > > 'valid' or 'empty' state in a class member and use it in size() and > > empty() instead of relying on Extent which is defined at overload > > resolution time and cannot be assigned at run time. > > I think this is a general issue with return default constructed objects > to indicate an error. The error is only defined implicitly. If > ControlList::get returns a 'Size' with width=0 and height=0, then it is > not clear if this indicates an error or if this value is really stored. Yes and no, if the custom type has a well defined interface it can be provided with an isValid()/isNull() function, like it is done for the Size class bool isNull() const { return !width && !height; } I understand that Size s = controlList.get(SomeSizeControl); if (s.isNull()) { /* Error out */ } Requires knowledge of the custom type interface and special care as it's less intuitive than handling an int return code or a pointer, but either we forbid return-by-value function signatures completely or we have to instrument custom types with an isValid() function for the caller to test. The Span class validity test is based on its Extent size, and the constructor you have introduced makes an invalid Span look like it is valid. > > You could add special class members to indicate that an object is > "valid", but this is not a nice solution and only works for custom types Other custom libcamera types are fine in that regard. I was suggesting to add a 'bool valid_;' class member to Span, to not rely on Extent to test its validity. > in libcamera. The "C" way would probably to indicate an error by a NULL > pointer and a returned error number. > > I think the proper way would be to throw an exception. This could also > be a custom libcamera exception. > Too bad libcamera doesn't use exceptions ;) > > > >> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count) > >> : data_(ptr) > >> { > >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > >> index 665bcac1..de8a7770 100644 > >> --- a/include/libcamera/controls.h > >> +++ b/include/libcamera/controls.h > >> @@ -167,7 +167,7 @@ public: > >> > >> using V = typename T::value_type; > >> const V *value = reinterpret_cast<const V *>(data().data()); > >> - return { value, numElements_ }; > >> + return T{ value, numElements_ }; > > > > this applies to overload > > > > template<typename T, typename std::enable_if_t<details::is_span<T>::value || > > std::is_same<std::string, std::remove_cv_t<T>>::value, > > T get() const > > { > > assert(type_ == details::control_type<std::remove_cv_t<T>>::value); > > assert(isArray_); > > > > using V = typename T::value_type; > > const V *value = reinterpret_cast<const V *>(data().data()); > > return { value, numElements_ }; > > } > > > > Isn't the returned { value, numElements_ } already used to construct an > > instance of T if assigned to an lvalue expression ? > > > > IOW why do you need to contruct an instance of T and return it > > explicitely ? Thanks to return value optimization this shouldn't have > > any performance impact but I have missed why it is needed. > > > > <2 minutes later> > > > > And now that I wrote this, if I remove it I get > > > > include/libcamera/controls.h:170:46: error: converting to > > ‘libcamera::Span<const float, 2>’ from initializer list would use > > explicit constructor ‘constexpr libcamera::Span<T, > > Extent>::Span(libcamera::Span<T, Extent>::pointer, libcamera::Span<T, > > Extent>::size_type) [with T = const float; long unsigned int Extent = > > 2; libcamera::Span<T, Extent>::pointer = const float*; > > libcamera::Span<T, Extent>::size_type = long unsigned int]’ > > > > Because indeed > > explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count) > > > > is, well, explicit (which I think it's ok and should not be removed) > > > > I'll leave here the above text anyway for others that might have the > > same question :) > > > >> } > >> > >> #ifndef __DOXYGEN__ > >> diff --git a/test/span.cpp b/test/span.cpp > >> index abf3a5d6..c37e2a66 100644 > >> --- a/test/span.cpp > >> +++ b/test/span.cpp > >> @@ -37,7 +37,7 @@ protected: > >> * to generate undefined behaviour. > >> */ > >> > >> - Span<int, 0>{}; > >> + /* Span<int, 0>{}; */ > > > > You should remove it, or if the above issue with constructing an > > 'empty' Span of size > 0 is resolved, prove that it works as intended. > > I commented it out instead of removing this, because the comment above > says "[...] Commented-out tests are expected not to compile [...]" and > there was already a commented case for an 4-sized Span. Anyway, I don't Oh I see, sorry, missed it. > see a real application of a 0-fixed-sized Span, as it will never be able > to store anything. My understanding is that it is only used for error conditions. Thanks j > > Best, > Christian > > > > > > Span<int, 4> s{} > > if (!s.empty()) > > error! > > > > Thanks > > j > > > >> /* Span<int, 4>{}; */ > >> > >> Span<int, 4>{ &i[0], 4 }; > >> -- > >> 2.25.1 > >>
Hi Jacopo, See my response below, specifically see the case about "std::optional". Am 02.04.22 um 10:46 schrieb Jacopo Mondi: > Hi Christian, > > On Sat, Apr 02, 2022 at 12:05:43AM +0100, Christian Rauch wrote: >> Hi Jacopo, >> >> Thanks for reviewing this. See my responses inline below. >> >> >> Am 01.04.22 um 15:28 schrieb Jacopo Mondi: >>> Hi Christian >>> >>> On Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote: >>>> The new default constructor allows to construct a fixed-sized Span via the >>>> default constructor of its stored data type. >>>> This prevents the construction of empty fixed-sized Spans that cannot hold >>>> any data. >>>> >>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> >>>> --- >>>> include/libcamera/base/span.h | 5 +++++ >>>> include/libcamera/controls.h | 2 +- >>>> test/span.cpp | 2 +- >>>> 3 files changed, 7 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h >>>> index 88d2e3de..7a4806dc 100644 >>>> --- a/include/libcamera/base/span.h >>>> +++ b/include/libcamera/base/span.h >>>> @@ -112,6 +112,11 @@ public: >>>> { >>>> } >>>> >>>> + Span() >>>> + { >>>> + Span(std::array<value_type, extent>{}); >>>> + } >>>> + >>> >>> This constructor creates a span of 'extent' elements all of them >>> initialized to 0 then ? >> >> All elements will be default constructed. Integers and floats will be >> initialised to 0, "std::strings" will be empty, and 'Size' and >> 'Rectangle' will use their specified default constructors. >> The default initialisation is required because the fixed-sized array >> cannot be empty. >> > > You're right, "initialized to 0" is not correct, "default constructed" > is the term I should have used :) > >>> >>> If I remove it I get >>> >>> ../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span<const float, 2>::Span(<brace-enclosed initializer list>)’ >>> 380 | return T{}; >>> >>> Caused by >>> template<typename T> >>> T get(const Control<T> &ctrl) const >>> { >>> const ControlValue *val = find(ctrl.id()); >>> if (!val) >>> return T{}; <------ >>> >>> return val->get<T>(); >>> } >>> >>> as now that extent is != 0 the expression doesn't bind anymore to the >>> existing constructor: >>> >>> template<bool Dependent = false, >>> typename = std::enable_if_t<Dependent || Extent == 0>> >>> constexpr Span() noexcept >>> : data_(nullptr) >>> { >>> } >>> >>> (I don't fully get what is the use of Dependent in the above definition :) >> >> I also do not understand the use of this constructor, since a >> 0-fixed-size Span cannot store anything. > > I think it was used to identify a non-valid span, to be returned in > case of errors > >>> >>> However I feel like creating a Span of Extent elements all zeroed is a >>> bit hill-defined, considering it is only used to return an error >>> condition. In example, if I'm not mistaken a Span constructed with >>> your proposed constructor will >>> >>> constexpr size_type size() const noexcept { return Extent; } >>> constexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); } >>> constexpr bool empty() const noexcept { return size() == 0; } >>> >>> be !empty and of size == 2, which is misleading considering we really >>> want to return an empty Span in case of error and the caller should >>> not be mis-leaded thinking it is valid. >>> >>> I don't have any super smart proposal to offer, if not recording the >>> 'valid' or 'empty' state in a class member and use it in size() and >>> empty() instead of relying on Extent which is defined at overload >>> resolution time and cannot be assigned at run time. >> >> I think this is a general issue with return default constructed objects >> to indicate an error. The error is only defined implicitly. If >> ControlList::get returns a 'Size' with width=0 and height=0, then it is >> not clear if this indicates an error or if this value is really stored. > > Yes and no, if the custom type has a well defined interface it can be > provided with an isValid()/isNull() function, like it is done for the > Size class > > bool isNull() const { return !width && !height; } > > I understand that > > Size s = controlList.get(SomeSizeControl); > if (s.isNull()) { > /* Error out */ > } > > Requires knowledge of the custom type interface and special care as > it's less intuitive than handling an int return code or a pointer, but > either we forbid return-by-value function signatures completely or we > have to instrument custom types with an isValid() function for the > caller to test. But this will only ever work for custom types. Basic C types and C++ standard types do not have such a flag. "isNull" only means that the area of the Size is zero, but it does not directly mean that the requested "Size" does not exist. > > The Span class validity test is based on its Extent size, and the > constructor you have introduced makes an invalid Span look like it is > valid. I really don't like this idea of implicitly encoding "validity" by a specific value :-) > >> >> You could add special class members to indicate that an object is >> "valid", but this is not a nice solution and only works for custom types > > Other custom libcamera types are fine in that regard. I was suggesting > to add a 'bool valid_;' class member to Span, to not rely on Extent to > test its validity. > >> in libcamera. The "C" way would probably to indicate an error by a NULL >> pointer and a returned error number. >> >> I think the proper way would be to throw an exception. This could also >> be a custom libcamera exception. >> > > Too bad libcamera doesn't use exceptions ;) Why is that? Has this something to do with "embedded C++"? Alternatively to exceptions, the get method could either 1) take a target value by reference and return a status flag (success / failure) or 2) take a status flag by reference and return a default constructed object. There is also "std::optional<>" which can express this: "A common use case for optional is the return value of a function that may fail." If you are ok with newer C++ features, than "optional" is maybe the most "expressive" solution: std::optional<Size> ret = controlList.get(SomeSizeControl); if (ret.has_value()) { // OK Size s = s.value(); } else { // ERROR } What do you think about this? The "optional" is essentially attaching a validity flag to any object returned by the "get" method, but it does not require changing the type itself. > >>> >>>> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count) >>>> : data_(ptr) >>>> { >>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >>>> index 665bcac1..de8a7770 100644 >>>> --- a/include/libcamera/controls.h >>>> +++ b/include/libcamera/controls.h >>>> @@ -167,7 +167,7 @@ public: >>>> >>>> using V = typename T::value_type; >>>> const V *value = reinterpret_cast<const V *>(data().data()); >>>> - return { value, numElements_ }; >>>> + return T{ value, numElements_ }; >>> >>> this applies to overload >>> >>> template<typename T, typename std::enable_if_t<details::is_span<T>::value || >>> std::is_same<std::string, std::remove_cv_t<T>>::value, >>> T get() const >>> { >>> assert(type_ == details::control_type<std::remove_cv_t<T>>::value); >>> assert(isArray_); >>> >>> using V = typename T::value_type; >>> const V *value = reinterpret_cast<const V *>(data().data()); >>> return { value, numElements_ }; >>> } >>> >>> Isn't the returned { value, numElements_ } already used to construct an >>> instance of T if assigned to an lvalue expression ? >>> >>> IOW why do you need to contruct an instance of T and return it >>> explicitely ? Thanks to return value optimization this shouldn't have >>> any performance impact but I have missed why it is needed. >>> >>> <2 minutes later> >>> >>> And now that I wrote this, if I remove it I get >>> >>> include/libcamera/controls.h:170:46: error: converting to >>> ‘libcamera::Span<const float, 2>’ from initializer list would use >>> explicit constructor ‘constexpr libcamera::Span<T, >>> Extent>::Span(libcamera::Span<T, Extent>::pointer, libcamera::Span<T, >>> Extent>::size_type) [with T = const float; long unsigned int Extent = >>> 2; libcamera::Span<T, Extent>::pointer = const float*; >>> libcamera::Span<T, Extent>::size_type = long unsigned int]’ >>> >>> Because indeed >>> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count) >>> >>> is, well, explicit (which I think it's ok and should not be removed) >>> >>> I'll leave here the above text anyway for others that might have the >>> same question :) >>> >>>> } >>>> >>>> #ifndef __DOXYGEN__ >>>> diff --git a/test/span.cpp b/test/span.cpp >>>> index abf3a5d6..c37e2a66 100644 >>>> --- a/test/span.cpp >>>> +++ b/test/span.cpp >>>> @@ -37,7 +37,7 @@ protected: >>>> * to generate undefined behaviour. >>>> */ >>>> >>>> - Span<int, 0>{}; >>>> + /* Span<int, 0>{}; */ >>> >>> You should remove it, or if the above issue with constructing an >>> 'empty' Span of size > 0 is resolved, prove that it works as intended. >> >> I commented it out instead of removing this, because the comment above >> says "[...] Commented-out tests are expected not to compile [...]" and >> there was already a commented case for an 4-sized Span. Anyway, I don't > > Oh I see, sorry, missed it. > >> see a real application of a 0-fixed-sized Span, as it will never be able >> to store anything. > > My understanding is that it is only used for error conditions. In this case, this "error condition" will be replaced by another compiler error that will trigger for the same reasons (empty span), but will be triggered by trying to construct an empty "std::array". Best, Christian > > Thanks > j > >> >> Best, >> Christian >> >> >>> >>> Span<int, 4> s{} >>> if (!s.empty()) >>> error! >>> >>> Thanks >>> j >>> >>>> /* Span<int, 4>{}; */ >>>> >>>> Span<int, 4>{ &i[0], 4 }; >>>> -- >>>> 2.25.1 >>>>
Hi Christian + Laurent On Sat, Apr 02, 2022 at 12:28:05PM +0100, Christian Rauch wrote: > Hi Jacopo, > > See my response below, specifically see the case about "std::optional". > > Am 02.04.22 um 10:46 schrieb Jacopo Mondi: > > Hi Christian, > > > > On Sat, Apr 02, 2022 at 12:05:43AM +0100, Christian Rauch wrote: > >> Hi Jacopo, > >> > >> Thanks for reviewing this. See my responses inline below. > >> > >> > >> Am 01.04.22 um 15:28 schrieb Jacopo Mondi: > >>> Hi Christian > >>> > >>> On Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote: > >>>> The new default constructor allows to construct a fixed-sized Span via the > >>>> default constructor of its stored data type. > >>>> This prevents the construction of empty fixed-sized Spans that cannot hold > >>>> any data. > >>>> > >>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> > >>>> --- > >>>> include/libcamera/base/span.h | 5 +++++ > >>>> include/libcamera/controls.h | 2 +- > >>>> test/span.cpp | 2 +- > >>>> 3 files changed, 7 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h > >>>> index 88d2e3de..7a4806dc 100644 > >>>> --- a/include/libcamera/base/span.h > >>>> +++ b/include/libcamera/base/span.h > >>>> @@ -112,6 +112,11 @@ public: > >>>> { > >>>> } > >>>> > >>>> + Span() > >>>> + { > >>>> + Span(std::array<value_type, extent>{}); > >>>> + } > >>>> + > >>> > >>> This constructor creates a span of 'extent' elements all of them > >>> initialized to 0 then ? > >> > >> All elements will be default constructed. Integers and floats will be > >> initialised to 0, "std::strings" will be empty, and 'Size' and > >> 'Rectangle' will use their specified default constructors. > >> The default initialisation is required because the fixed-sized array > >> cannot be empty. > >> > > > > You're right, "initialized to 0" is not correct, "default constructed" > > is the term I should have used :) > > > >>> > >>> If I remove it I get > >>> > >>> ../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span<const float, 2>::Span(<brace-enclosed initializer list>)’ > >>> 380 | return T{}; > >>> > >>> Caused by > >>> template<typename T> > >>> T get(const Control<T> &ctrl) const > >>> { > >>> const ControlValue *val = find(ctrl.id()); > >>> if (!val) > >>> return T{}; <------ > >>> > >>> return val->get<T>(); > >>> } > >>> > >>> as now that extent is != 0 the expression doesn't bind anymore to the > >>> existing constructor: > >>> > >>> template<bool Dependent = false, > >>> typename = std::enable_if_t<Dependent || Extent == 0>> > >>> constexpr Span() noexcept > >>> : data_(nullptr) > >>> { > >>> } > >>> > >>> (I don't fully get what is the use of Dependent in the above definition :) > >> > >> I also do not understand the use of this constructor, since a > >> 0-fixed-size Span cannot store anything. > > > > I think it was used to identify a non-valid span, to be returned in > > case of errors > > > >>> > >>> However I feel like creating a Span of Extent elements all zeroed is a > >>> bit hill-defined, considering it is only used to return an error > >>> condition. In example, if I'm not mistaken a Span constructed with > >>> your proposed constructor will > >>> > >>> constexpr size_type size() const noexcept { return Extent; } > >>> constexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); } > >>> constexpr bool empty() const noexcept { return size() == 0; } > >>> > >>> be !empty and of size == 2, which is misleading considering we really > >>> want to return an empty Span in case of error and the caller should > >>> not be mis-leaded thinking it is valid. > >>> > >>> I don't have any super smart proposal to offer, if not recording the > >>> 'valid' or 'empty' state in a class member and use it in size() and > >>> empty() instead of relying on Extent which is defined at overload > >>> resolution time and cannot be assigned at run time. > >> > >> I think this is a general issue with return default constructed objects > >> to indicate an error. The error is only defined implicitly. If > >> ControlList::get returns a 'Size' with width=0 and height=0, then it is > >> not clear if this indicates an error or if this value is really stored. > > > > Yes and no, if the custom type has a well defined interface it can be > > provided with an isValid()/isNull() function, like it is done for the > > Size class > > > > bool isNull() const { return !width && !height; } > > > > I understand that > > > > Size s = controlList.get(SomeSizeControl); > > if (s.isNull()) { > > /* Error out */ > > } > > > > Requires knowledge of the custom type interface and special care as > > it's less intuitive than handling an int return code or a pointer, but > > either we forbid return-by-value function signatures completely or we > > have to instrument custom types with an isValid() function for the > > caller to test. > > But this will only ever work for custom types. Basic C types and C++ > standard types do not have such a flag. "isNull" only means that the > area of the Size is zero, but it does not directly mean that the > requested "Size" does not exist. > To me standard type are less a concern as they all have functions to test if they're empty. Of course this assumes an empty vector returned by a function is an error condition, something that it's not semantically accurate, I agree > > > > The Span class validity test is based on its Extent size, and the > > constructor you have introduced makes an invalid Span look like it is > > valid. > > I really don't like this idea of implicitly encoding "validity" by a > specific value :-) > > > > >> > >> You could add special class members to indicate that an object is > >> "valid", but this is not a nice solution and only works for custom types > > > > Other custom libcamera types are fine in that regard. I was suggesting > > to add a 'bool valid_;' class member to Span, to not rely on Extent to > > test its validity. > > > >> in libcamera. The "C" way would probably to indicate an error by a NULL > >> pointer and a returned error number. > >> > >> I think the proper way would be to throw an exception. This could also > >> be a custom libcamera exception. > >> > > > > Too bad libcamera doesn't use exceptions ;) > > Why is that? Has this something to do with "embedded C++"? Alternatively I think among the reasons, apart from code size and efficiency on which I don't have numbers to debate on but seems sensible for some embedded platforms, there is the fact that using exceptions would require code that use libcamera to be exception-safe, it would have made more complex creating a C-API and deal with errors through IPC (which libcamera use heavily to communicate with IPA modules). > to exceptions, the get method could either 1) take a target value by > reference and return a status flag (success / failure) or 2) take a > status flag by reference and return a default constructed object. > > There is also "std::optional<>" which can express this: "A common use > case for optional is the return value of a function that may fail." > > If you are ok with newer C++ features, than "optional" is maybe the most > "expressive" solution: > > std::optional<Size> ret = controlList.get(SomeSizeControl); > if (ret.has_value()) { > // OK > Size s = s.value(); > } > else { > // ERROR > } > > What do you think about this? The "optional" is essentially attaching a > validity flag to any object returned by the "get" method, but it does > not require changing the type itself. > std::optional<> has been introduced in C++17, which libcamera migrated to only 1.5 year ago. At the time the Control interface had been designed std::optional<> was not available, if memory does not fail me. However it seems a rather neat way to handle return-by-value error conditions and I would be in favour to move the code base to use it. We already have a user in libcamera, introduced by RPi when adding support to color space handling include/libcamera/internal/v4l2_device.h: static std::optional<ColorSpace> toColorSpace(const T &v4l2Format); Let me rope-in Laurent by CC-ing him to know what he thinks. Thanks for the discussion starter! > > > >>> > >>>> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count) > >>>> : data_(ptr) > >>>> { > >>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > >>>> index 665bcac1..de8a7770 100644 > >>>> --- a/include/libcamera/controls.h > >>>> +++ b/include/libcamera/controls.h > >>>> @@ -167,7 +167,7 @@ public: > >>>> > >>>> using V = typename T::value_type; > >>>> const V *value = reinterpret_cast<const V *>(data().data()); > >>>> - return { value, numElements_ }; > >>>> + return T{ value, numElements_ }; > >>> > >>> this applies to overload > >>> > >>> template<typename T, typename std::enable_if_t<details::is_span<T>::value || > >>> std::is_same<std::string, std::remove_cv_t<T>>::value, > >>> T get() const > >>> { > >>> assert(type_ == details::control_type<std::remove_cv_t<T>>::value); > >>> assert(isArray_); > >>> > >>> using V = typename T::value_type; > >>> const V *value = reinterpret_cast<const V *>(data().data()); > >>> return { value, numElements_ }; > >>> } > >>> > >>> Isn't the returned { value, numElements_ } already used to construct an > >>> instance of T if assigned to an lvalue expression ? > >>> > >>> IOW why do you need to contruct an instance of T and return it > >>> explicitely ? Thanks to return value optimization this shouldn't have > >>> any performance impact but I have missed why it is needed. > >>> > >>> <2 minutes later> > >>> > >>> And now that I wrote this, if I remove it I get > >>> > >>> include/libcamera/controls.h:170:46: error: converting to > >>> ‘libcamera::Span<const float, 2>’ from initializer list would use > >>> explicit constructor ‘constexpr libcamera::Span<T, > >>> Extent>::Span(libcamera::Span<T, Extent>::pointer, libcamera::Span<T, > >>> Extent>::size_type) [with T = const float; long unsigned int Extent = > >>> 2; libcamera::Span<T, Extent>::pointer = const float*; > >>> libcamera::Span<T, Extent>::size_type = long unsigned int]’ > >>> > >>> Because indeed > >>> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count) > >>> > >>> is, well, explicit (which I think it's ok and should not be removed) > >>> > >>> I'll leave here the above text anyway for others that might have the > >>> same question :) > >>> > >>>> } > >>>> > >>>> #ifndef __DOXYGEN__ > >>>> diff --git a/test/span.cpp b/test/span.cpp > >>>> index abf3a5d6..c37e2a66 100644 > >>>> --- a/test/span.cpp > >>>> +++ b/test/span.cpp > >>>> @@ -37,7 +37,7 @@ protected: > >>>> * to generate undefined behaviour. > >>>> */ > >>>> > >>>> - Span<int, 0>{}; > >>>> + /* Span<int, 0>{}; */ > >>> > >>> You should remove it, or if the above issue with constructing an > >>> 'empty' Span of size > 0 is resolved, prove that it works as intended. > >> > >> I commented it out instead of removing this, because the comment above > >> says "[...] Commented-out tests are expected not to compile [...]" and > >> there was already a commented case for an 4-sized Span. Anyway, I don't > > > > Oh I see, sorry, missed it. > > > >> see a real application of a 0-fixed-sized Span, as it will never be able > >> to store anything. > > > > My understanding is that it is only used for error conditions. > > In this case, this "error condition" will be replaced by another > compiler error that will trigger for the same reasons (empty span), but > will be triggered by trying to construct an empty "std::array". > > Best, > Christian > > > > > Thanks > > j > > > >> > >> Best, > >> Christian > >> > >> > >>> > >>> Span<int, 4> s{} > >>> if (!s.empty()) > >>> error! > >>> > >>> Thanks > >>> j > >>> > >>>> /* Span<int, 4>{}; */ > >>>> > >>>> Span<int, 4>{ &i[0], 4 }; > >>>> -- > >>>> 2.25.1 > >>>>
Hello, On Mon, Apr 04, 2022 at 12:32:47PM +0200, Jacopo Mondi wrote: > On Sat, Apr 02, 2022 at 12:28:05PM +0100, Christian Rauch wrote: > > Hi Jacopo, > > > > See my response below, specifically see the case about "std::optional". > > > > Am 02.04.22 um 10:46 schrieb Jacopo Mondi: > > > On Sat, Apr 02, 2022 at 12:05:43AM +0100, Christian Rauch wrote: > > >> Am 01.04.22 um 15:28 schrieb Jacopo Mondi: > > >>> On Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote: > > >>>> The new default constructor allows to construct a fixed-sized Span via the > > >>>> default constructor of its stored data type. > > >>>> This prevents the construction of empty fixed-sized Spans that cannot hold > > >>>> any data. > > >>>> > > >>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> > > >>>> --- > > >>>> include/libcamera/base/span.h | 5 +++++ > > >>>> include/libcamera/controls.h | 2 +- > > >>>> test/span.cpp | 2 +- > > >>>> 3 files changed, 7 insertions(+), 2 deletions(-) > > >>>> > > >>>> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h > > >>>> index 88d2e3de..7a4806dc 100644 > > >>>> --- a/include/libcamera/base/span.h > > >>>> +++ b/include/libcamera/base/span.h > > >>>> @@ -112,6 +112,11 @@ public: > > >>>> { > > >>>> } > > >>>> > > >>>> + Span() > > >>>> + { > > >>>> + Span(std::array<value_type, extent>{}); I don't think this will do what you expect. The code is equivalent to std::array<value_type, extent> data{}; Span(data); The first line allocates an array of extent default-initialized elements on the stack. The second line calls the Span constructor that takes an std::array argument, and the span's data_ pointer points to array.data(). Then, when you return from the destructor, the array variable is destroyed, and data_ points to freed memory. The Span class is modelled after std::span, introduced in C++20. When libcamera will move to C++20 (I don't expect that to happen before a few years), the plan is to switch to std::span. We should thus keep the Span implementation as close as possible to std::span (it can't be exactly identical, as std::span relies on other C++20 features, for instance constructor 7 in [1] uses the C++20 range API, so we have two different constructors that take a reference to a container instead). [1] https://en.cppreference.com/w/cpp/container/span/span > > >>>> + } > > >>>> + > > >>> > > >>> This constructor creates a span of 'extent' elements all of them > > >>> initialized to 0 then ? > > >> > > >> All elements will be default constructed. Integers and floats will be > > >> initialised to 0, "std::strings" will be empty, and 'Size' and > > >> 'Rectangle' will use their specified default constructors. > > >> The default initialisation is required because the fixed-sized array > > >> cannot be empty. > > > > > > You're right, "initialized to 0" is not correct, "default constructed" > > > is the term I should have used :) > > > > > >>> If I remove it I get > > >>> > > >>> ../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span<const float, 2>::Span(<brace-enclosed initializer list>)’ > > >>> 380 | return T{}; > > >>> > > >>> Caused by > > >>> template<typename T> > > >>> T get(const Control<T> &ctrl) const > > >>> { > > >>> const ControlValue *val = find(ctrl.id()); > > >>> if (!val) > > >>> return T{}; <------ > > >>> > > >>> return val->get<T>(); > > >>> } > > >>> > > >>> as now that extent is != 0 the expression doesn't bind anymore to the > > >>> existing constructor: > > >>> > > >>> template<bool Dependent = false, > > >>> typename = std::enable_if_t<Dependent || Extent == 0>> > > >>> constexpr Span() noexcept > > >>> : data_(nullptr) > > >>> { > > >>> } > > >>> > > >>> (I don't fully get what is the use of Dependent in the above definition :) > > >> > > >> I also do not understand the use of this constructor, since a > > >> 0-fixed-size Span cannot store anything. > > > > > > I think it was used to identify a non-valid span, to be returned in > > > case of errors > > > > > >>> However I feel like creating a Span of Extent elements all zeroed is a > > >>> bit hill-defined, considering it is only used to return an error > > >>> condition. In example, if I'm not mistaken a Span constructed with > > >>> your proposed constructor will > > >>> > > >>> constexpr size_type size() const noexcept { return Extent; } > > >>> constexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); } > > >>> constexpr bool empty() const noexcept { return size() == 0; } > > >>> > > >>> be !empty and of size == 2, which is misleading considering we really > > >>> want to return an empty Span in case of error and the caller should > > >>> not be mis-leaded thinking it is valid. > > >>> > > >>> I don't have any super smart proposal to offer, if not recording the > > >>> 'valid' or 'empty' state in a class member and use it in size() and > > >>> empty() instead of relying on Extent which is defined at overload > > >>> resolution time and cannot be assigned at run time. > > >> > > >> I think this is a general issue with return default constructed objects > > >> to indicate an error. The error is only defined implicitly. If > > >> ControlList::get returns a 'Size' with width=0 and height=0, then it is > > >> not clear if this indicates an error or if this value is really stored. > > > > > > Yes and no, if the custom type has a well defined interface it can be > > > provided with an isValid()/isNull() function, like it is done for the > > > Size class > > > > > > bool isNull() const { return !width && !height; } > > > > > > I understand that > > > > > > Size s = controlList.get(SomeSizeControl); > > > if (s.isNull()) { > > > /* Error out */ > > > } > > > > > > Requires knowledge of the custom type interface and special care as > > > it's less intuitive than handling an int return code or a pointer, but > > > either we forbid return-by-value function signatures completely or we > > > have to instrument custom types with an isValid() function for the > > > caller to test. > > > > But this will only ever work for custom types. Basic C types and C++ > > standard types do not have such a flag. "isNull" only means that the > > area of the Size is zero, but it does not directly mean that the > > requested "Size" does not exist. > > To me standard type are less a concern as they all have functions to test if > they're empty. Of course this assumes an empty vector returned by a > function is an error condition, something that it's not semantically > accurate, I agree > > > > The Span class validity test is based on its Extent size, and the > > > constructor you have introduced makes an invalid Span look like it is > > > valid. > > > > I really don't like this idea of implicitly encoding "validity" by a > > specific value :-) > > > > >> You could add special class members to indicate that an object is > > >> "valid", but this is not a nice solution and only works for custom types > > > > > > Other custom libcamera types are fine in that regard. I was suggesting > > > to add a 'bool valid_;' class member to Span, to not rely on Extent to > > > test its validity. > > > > > >> in libcamera. The "C" way would probably to indicate an error by a NULL > > >> pointer and a returned error number. > > >> > > >> I think the proper way would be to throw an exception. This could also > > >> be a custom libcamera exception. > > > > > > Too bad libcamera doesn't use exceptions ;) > > > > Why is that? Has this something to do with "embedded C++"? Alternatively > > I think among the reasons, apart from code size and efficiency on > which I don't have numbers to debate on but seems sensible for some > embedded platforms, there is the fact that using exceptions > would require code that use libcamera to be exception-safe, it would > have made more complex creating a C-API and deal with errors through > IPC (which libcamera use heavily to communicate with IPA modules). > > > to exceptions, the get method could either 1) take a target value by > > reference and return a status flag (success / failure) or 2) take a > > status flag by reference and return a default constructed object. > > > > There is also "std::optional<>" which can express this: "A common use > > case for optional is the return value of a function that may fail." > > > > If you are ok with newer C++ features, than "optional" is maybe the most > > "expressive" solution: > > > > std::optional<Size> ret = controlList.get(SomeSizeControl); > > if (ret.has_value()) { > > // OK > > Size s = s.value(); > > } > > else { > > // ERROR > > } > > > > What do you think about this? The "optional" is essentially attaching a > > validity flag to any object returned by the "get" method, but it does > > not require changing the type itself. > > std::optional<> has been introduced in C++17, which libcamera migrated > to only 1.5 year ago. At the time the Control interface had been > designed std::optional<> was not available, if memory does not fail me. > > However it seems a rather neat way to handle return-by-value error > conditions and I would be in favour to move the code base to use it. I do like std::optional, it's a useful feature. We use it internally, and we can extend its usage. However, while libcamera is compiled for C++17, we keep the public API C++14-compatible. This was decided to allow applications still using C++14 to compile and link against libcamera, with the main use case being Chromium (for which we have implement native libcamera support in [2], not merged in mainline yet). It seems that Chromium has now moved to C++17 ([3]), so we could possibly follow suit. We'll have to rebuild and test Chromium integration on the latest version first. This being said, I would prefer keeping the public API C++14-compatible for a bit longer if possible, as not all applications have moved to C++17. If C++17 feature would be so useful in the public API that dropping support for C++14 users would be an acceptable drawback, I'm not against that [2] https://github.com/libcamera-org/chromium [3] https://bugs.chromium.org/p/chromium/issues/detail?id=752720 > We already have a user in libcamera, introduced by RPi when adding > support to color space handling > include/libcamera/internal/v4l2_device.h: static std::optional<ColorSpace> toColorSpace(const T &v4l2Format); > > Let me rope-in Laurent by CC-ing him to know what he thinks. > > Thanks for the discussion starter! > > > >>>> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count) > > >>>> : data_(ptr) > > >>>> { > > >>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > >>>> index 665bcac1..de8a7770 100644 > > >>>> --- a/include/libcamera/controls.h > > >>>> +++ b/include/libcamera/controls.h > > >>>> @@ -167,7 +167,7 @@ public: > > >>>> > > >>>> using V = typename T::value_type; > > >>>> const V *value = reinterpret_cast<const V *>(data().data()); > > >>>> - return { value, numElements_ }; > > >>>> + return T{ value, numElements_ }; > > >>> > > >>> this applies to overload > > >>> > > >>> template<typename T, typename std::enable_if_t<details::is_span<T>::value || > > >>> std::is_same<std::string, std::remove_cv_t<T>>::value, > > >>> T get() const > > >>> { > > >>> assert(type_ == details::control_type<std::remove_cv_t<T>>::value); > > >>> assert(isArray_); > > >>> > > >>> using V = typename T::value_type; > > >>> const V *value = reinterpret_cast<const V *>(data().data()); > > >>> return { value, numElements_ }; > > >>> } > > >>> > > >>> Isn't the returned { value, numElements_ } already used to construct an > > >>> instance of T if assigned to an lvalue expression ? > > >>> > > >>> IOW why do you need to contruct an instance of T and return it > > >>> explicitely ? Thanks to return value optimization this shouldn't have > > >>> any performance impact but I have missed why it is needed. > > >>> > > >>> <2 minutes later> > > >>> > > >>> And now that I wrote this, if I remove it I get > > >>> > > >>> include/libcamera/controls.h:170:46: error: converting to > > >>> ‘libcamera::Span<const float, 2>’ from initializer list would use > > >>> explicit constructor ‘constexpr libcamera::Span<T, > > >>> Extent>::Span(libcamera::Span<T, Extent>::pointer, libcamera::Span<T, > > >>> Extent>::size_type) [with T = const float; long unsigned int Extent = > > >>> 2; libcamera::Span<T, Extent>::pointer = const float*; > > >>> libcamera::Span<T, Extent>::size_type = long unsigned int]’ > > >>> > > >>> Because indeed > > >>> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count) > > >>> > > >>> is, well, explicit (which I think it's ok and should not be removed) > > >>> > > >>> I'll leave here the above text anyway for others that might have the > > >>> same question :) > > >>> > > >>>> } > > >>>> > > >>>> #ifndef __DOXYGEN__ > > >>>> diff --git a/test/span.cpp b/test/span.cpp > > >>>> index abf3a5d6..c37e2a66 100644 > > >>>> --- a/test/span.cpp > > >>>> +++ b/test/span.cpp > > >>>> @@ -37,7 +37,7 @@ protected: > > >>>> * to generate undefined behaviour. > > >>>> */ > > >>>> > > >>>> - Span<int, 0>{}; > > >>>> + /* Span<int, 0>{}; */ > > >>> > > >>> You should remove it, or if the above issue with constructing an > > >>> 'empty' Span of size > 0 is resolved, prove that it works as intended. > > >> > > >> I commented it out instead of removing this, because the comment above > > >> says "[...] Commented-out tests are expected not to compile [...]" and > > >> there was already a commented case for an 4-sized Span. Anyway, I don't > > > > > > Oh I see, sorry, missed it. > > > > > >> see a real application of a 0-fixed-sized Span, as it will never be able > > >> to store anything. > > > > > > My understanding is that it is only used for error conditions. > > > > In this case, this "error condition" will be replaced by another > > compiler error that will trigger for the same reasons (empty span), but > > will be triggered by trying to construct an empty "std::array". > > > > >>> Span<int, 4> s{} > > >>> if (!s.empty()) > > >>> error! > > >>> > > >>>> /* Span<int, 4>{}; */ > > >>>> > > >>>> Span<int, 4>{ &i[0], 4 };
Dear Laurent, Thanks for pointing out that the "std::array" in my patch is only temporarily constructed. I overlooked that the Span only stores the pointer to the data and the size, and not the actual data. This, and other minor issues, have been fixed in version 2 of my patch set. Best, Christian Am 04.04.22 um 12:32 schrieb Laurent Pinchart: > Hello, > > On Mon, Apr 04, 2022 at 12:32:47PM +0200, Jacopo Mondi wrote: >> On Sat, Apr 02, 2022 at 12:28:05PM +0100, Christian Rauch wrote: >>> Hi Jacopo, >>> >>> See my response below, specifically see the case about "std::optional". >>> >>> Am 02.04.22 um 10:46 schrieb Jacopo Mondi: >>>> On Sat, Apr 02, 2022 at 12:05:43AM +0100, Christian Rauch wrote: >>>>> Am 01.04.22 um 15:28 schrieb Jacopo Mondi: >>>>>> On Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote: >>>>>>> The new default constructor allows to construct a fixed-sized Span via the >>>>>>> default constructor of its stored data type. >>>>>>> This prevents the construction of empty fixed-sized Spans that cannot hold >>>>>>> any data. >>>>>>> >>>>>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> >>>>>>> --- >>>>>>> include/libcamera/base/span.h | 5 +++++ >>>>>>> include/libcamera/controls.h | 2 +- >>>>>>> test/span.cpp | 2 +- >>>>>>> 3 files changed, 7 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h >>>>>>> index 88d2e3de..7a4806dc 100644 >>>>>>> --- a/include/libcamera/base/span.h >>>>>>> +++ b/include/libcamera/base/span.h >>>>>>> @@ -112,6 +112,11 @@ public: >>>>>>> { >>>>>>> } >>>>>>> >>>>>>> + Span() >>>>>>> + { >>>>>>> + Span(std::array<value_type, extent>{}); > > I don't think this will do what you expect. The code is equivalent to > > std::array<value_type, extent> data{}; > > Span(data); > > The first line allocates an array of extent default-initialized elements > on the stack. The second line calls the Span constructor that takes an > std::array argument, and the span's data_ pointer points to > array.data(). Then, when you return from the destructor, the array > variable is destroyed, and data_ points to freed memory. > > The Span class is modelled after std::span, introduced in C++20. When > libcamera will move to C++20 (I don't expect that to happen before a few > years), the plan is to switch to std::span. We should thus keep the Span > implementation as close as possible to std::span (it can't be exactly > identical, as std::span relies on other C++20 features, for instance > constructor 7 in [1] uses the C++20 range API, so we have two different > constructors that take a reference to a container instead). > > [1] https://en.cppreference.com/w/cpp/container/span/span > >>>>>>> + } >>>>>>> + >>>>>> >>>>>> This constructor creates a span of 'extent' elements all of them >>>>>> initialized to 0 then ? >>>>> >>>>> All elements will be default constructed. Integers and floats will be >>>>> initialised to 0, "std::strings" will be empty, and 'Size' and >>>>> 'Rectangle' will use their specified default constructors. >>>>> The default initialisation is required because the fixed-sized array >>>>> cannot be empty. >>>> >>>> You're right, "initialized to 0" is not correct, "default constructed" >>>> is the term I should have used :) >>>> >>>>>> If I remove it I get >>>>>> >>>>>> ../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span<const float, 2>::Span(<brace-enclosed initializer list>)’ >>>>>> 380 | return T{}; >>>>>> >>>>>> Caused by >>>>>> template<typename T> >>>>>> T get(const Control<T> &ctrl) const >>>>>> { >>>>>> const ControlValue *val = find(ctrl.id()); >>>>>> if (!val) >>>>>> return T{}; <------ >>>>>> >>>>>> return val->get<T>(); >>>>>> } >>>>>> >>>>>> as now that extent is != 0 the expression doesn't bind anymore to the >>>>>> existing constructor: >>>>>> >>>>>> template<bool Dependent = false, >>>>>> typename = std::enable_if_t<Dependent || Extent == 0>> >>>>>> constexpr Span() noexcept >>>>>> : data_(nullptr) >>>>>> { >>>>>> } >>>>>> >>>>>> (I don't fully get what is the use of Dependent in the above definition :) >>>>> >>>>> I also do not understand the use of this constructor, since a >>>>> 0-fixed-size Span cannot store anything. >>>> >>>> I think it was used to identify a non-valid span, to be returned in >>>> case of errors >>>> >>>>>> However I feel like creating a Span of Extent elements all zeroed is a >>>>>> bit hill-defined, considering it is only used to return an error >>>>>> condition. In example, if I'm not mistaken a Span constructed with >>>>>> your proposed constructor will >>>>>> >>>>>> constexpr size_type size() const noexcept { return Extent; } >>>>>> constexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); } >>>>>> constexpr bool empty() const noexcept { return size() == 0; } >>>>>> >>>>>> be !empty and of size == 2, which is misleading considering we really >>>>>> want to return an empty Span in case of error and the caller should >>>>>> not be mis-leaded thinking it is valid. >>>>>> >>>>>> I don't have any super smart proposal to offer, if not recording the >>>>>> 'valid' or 'empty' state in a class member and use it in size() and >>>>>> empty() instead of relying on Extent which is defined at overload >>>>>> resolution time and cannot be assigned at run time. >>>>> >>>>> I think this is a general issue with return default constructed objects >>>>> to indicate an error. The error is only defined implicitly. If >>>>> ControlList::get returns a 'Size' with width=0 and height=0, then it is >>>>> not clear if this indicates an error or if this value is really stored. >>>> >>>> Yes and no, if the custom type has a well defined interface it can be >>>> provided with an isValid()/isNull() function, like it is done for the >>>> Size class >>>> >>>> bool isNull() const { return !width && !height; } >>>> >>>> I understand that >>>> >>>> Size s = controlList.get(SomeSizeControl); >>>> if (s.isNull()) { >>>> /* Error out */ >>>> } >>>> >>>> Requires knowledge of the custom type interface and special care as >>>> it's less intuitive than handling an int return code or a pointer, but >>>> either we forbid return-by-value function signatures completely or we >>>> have to instrument custom types with an isValid() function for the >>>> caller to test. >>> >>> But this will only ever work for custom types. Basic C types and C++ >>> standard types do not have such a flag. "isNull" only means that the >>> area of the Size is zero, but it does not directly mean that the >>> requested "Size" does not exist. >> >> To me standard type are less a concern as they all have functions to test if >> they're empty. Of course this assumes an empty vector returned by a >> function is an error condition, something that it's not semantically >> accurate, I agree >> >>>> The Span class validity test is based on its Extent size, and the >>>> constructor you have introduced makes an invalid Span look like it is >>>> valid. >>> >>> I really don't like this idea of implicitly encoding "validity" by a >>> specific value :-) >>> >>>>> You could add special class members to indicate that an object is >>>>> "valid", but this is not a nice solution and only works for custom types >>>> >>>> Other custom libcamera types are fine in that regard. I was suggesting >>>> to add a 'bool valid_;' class member to Span, to not rely on Extent to >>>> test its validity. >>>> >>>>> in libcamera. The "C" way would probably to indicate an error by a NULL >>>>> pointer and a returned error number. >>>>> >>>>> I think the proper way would be to throw an exception. This could also >>>>> be a custom libcamera exception. >>>> >>>> Too bad libcamera doesn't use exceptions ;) >>> >>> Why is that? Has this something to do with "embedded C++"? Alternatively >> >> I think among the reasons, apart from code size and efficiency on >> which I don't have numbers to debate on but seems sensible for some >> embedded platforms, there is the fact that using exceptions >> would require code that use libcamera to be exception-safe, it would >> have made more complex creating a C-API and deal with errors through >> IPC (which libcamera use heavily to communicate with IPA modules). >> >>> to exceptions, the get method could either 1) take a target value by >>> reference and return a status flag (success / failure) or 2) take a >>> status flag by reference and return a default constructed object. >>> >>> There is also "std::optional<>" which can express this: "A common use >>> case for optional is the return value of a function that may fail." >>> >>> If you are ok with newer C++ features, than "optional" is maybe the most >>> "expressive" solution: >>> >>> std::optional<Size> ret = controlList.get(SomeSizeControl); >>> if (ret.has_value()) { >>> // OK >>> Size s = s.value(); >>> } >>> else { >>> // ERROR >>> } >>> >>> What do you think about this? The "optional" is essentially attaching a >>> validity flag to any object returned by the "get" method, but it does >>> not require changing the type itself. >> >> std::optional<> has been introduced in C++17, which libcamera migrated >> to only 1.5 year ago. At the time the Control interface had been >> designed std::optional<> was not available, if memory does not fail me. >> >> However it seems a rather neat way to handle return-by-value error >> conditions and I would be in favour to move the code base to use it. > > I do like std::optional, it's a useful feature. We use it internally, > and we can extend its usage. However, while libcamera is compiled for > C++17, we keep the public API C++14-compatible. This was decided to > allow applications still using C++14 to compile and link against > libcamera, with the main use case being Chromium (for which we have > implement native libcamera support in [2], not merged in mainline yet). > It seems that Chromium has now moved to C++17 ([3]), so we could > possibly follow suit. We'll have to rebuild and test Chromium > integration on the latest version first. > > This being said, I would prefer keeping the public API C++14-compatible > for a bit longer if possible, as not all applications have moved to > C++17. If C++17 feature would be so useful in the public API that > dropping support for C++14 users would be an acceptable drawback, I'm > not against that > > [2] https://github.com/libcamera-org/chromium > [3] https://bugs.chromium.org/p/chromium/issues/detail?id=752720 > >> We already have a user in libcamera, introduced by RPi when adding >> support to color space handling >> include/libcamera/internal/v4l2_device.h: static std::optional<ColorSpace> toColorSpace(const T &v4l2Format); >> >> Let me rope-in Laurent by CC-ing him to know what he thinks. >> >> Thanks for the discussion starter! >> >>>>>>> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count) >>>>>>> : data_(ptr) >>>>>>> { >>>>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >>>>>>> index 665bcac1..de8a7770 100644 >>>>>>> --- a/include/libcamera/controls.h >>>>>>> +++ b/include/libcamera/controls.h >>>>>>> @@ -167,7 +167,7 @@ public: >>>>>>> >>>>>>> using V = typename T::value_type; >>>>>>> const V *value = reinterpret_cast<const V *>(data().data()); >>>>>>> - return { value, numElements_ }; >>>>>>> + return T{ value, numElements_ }; >>>>>> >>>>>> this applies to overload >>>>>> >>>>>> template<typename T, typename std::enable_if_t<details::is_span<T>::value || >>>>>> std::is_same<std::string, std::remove_cv_t<T>>::value, >>>>>> T get() const >>>>>> { >>>>>> assert(type_ == details::control_type<std::remove_cv_t<T>>::value); >>>>>> assert(isArray_); >>>>>> >>>>>> using V = typename T::value_type; >>>>>> const V *value = reinterpret_cast<const V *>(data().data()); >>>>>> return { value, numElements_ }; >>>>>> } >>>>>> >>>>>> Isn't the returned { value, numElements_ } already used to construct an >>>>>> instance of T if assigned to an lvalue expression ? >>>>>> >>>>>> IOW why do you need to contruct an instance of T and return it >>>>>> explicitely ? Thanks to return value optimization this shouldn't have >>>>>> any performance impact but I have missed why it is needed. >>>>>> >>>>>> <2 minutes later> >>>>>> >>>>>> And now that I wrote this, if I remove it I get >>>>>> >>>>>> include/libcamera/controls.h:170:46: error: converting to >>>>>> ‘libcamera::Span<const float, 2>’ from initializer list would use >>>>>> explicit constructor ‘constexpr libcamera::Span<T, >>>>>> Extent>::Span(libcamera::Span<T, Extent>::pointer, libcamera::Span<T, >>>>>> Extent>::size_type) [with T = const float; long unsigned int Extent = >>>>>> 2; libcamera::Span<T, Extent>::pointer = const float*; >>>>>> libcamera::Span<T, Extent>::size_type = long unsigned int]’ >>>>>> >>>>>> Because indeed >>>>>> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count) >>>>>> >>>>>> is, well, explicit (which I think it's ok and should not be removed) >>>>>> >>>>>> I'll leave here the above text anyway for others that might have the >>>>>> same question :) >>>>>> >>>>>>> } >>>>>>> >>>>>>> #ifndef __DOXYGEN__ >>>>>>> diff --git a/test/span.cpp b/test/span.cpp >>>>>>> index abf3a5d6..c37e2a66 100644 >>>>>>> --- a/test/span.cpp >>>>>>> +++ b/test/span.cpp >>>>>>> @@ -37,7 +37,7 @@ protected: >>>>>>> * to generate undefined behaviour. >>>>>>> */ >>>>>>> >>>>>>> - Span<int, 0>{}; >>>>>>> + /* Span<int, 0>{}; */ >>>>>> >>>>>> You should remove it, or if the above issue with constructing an >>>>>> 'empty' Span of size > 0 is resolved, prove that it works as intended. >>>>> >>>>> I commented it out instead of removing this, because the comment above >>>>> says "[...] Commented-out tests are expected not to compile [...]" and >>>>> there was already a commented case for an 4-sized Span. Anyway, I don't >>>> >>>> Oh I see, sorry, missed it. >>>> >>>>> see a real application of a 0-fixed-sized Span, as it will never be able >>>>> to store anything. >>>> >>>> My understanding is that it is only used for error conditions. >>> >>> In this case, this "error condition" will be replaced by another >>> compiler error that will trigger for the same reasons (empty span), but >>> will be triggered by trying to construct an empty "std::array". >>> >>>>>> Span<int, 4> s{} >>>>>> if (!s.empty()) >>>>>> error! >>>>>> >>>>>>> /* Span<int, 4>{}; */ >>>>>>> >>>>>>> Span<int, 4>{ &i[0], 4 }; >
diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h index 88d2e3de..7a4806dc 100644 --- a/include/libcamera/base/span.h +++ b/include/libcamera/base/span.h @@ -112,6 +112,11 @@ public: { } + Span() + { + Span(std::array<value_type, extent>{}); + } + explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count) : data_(ptr) { diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index 665bcac1..de8a7770 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -167,7 +167,7 @@ public: using V = typename T::value_type; const V *value = reinterpret_cast<const V *>(data().data()); - return { value, numElements_ }; + return T{ value, numElements_ }; } #ifndef __DOXYGEN__ diff --git a/test/span.cpp b/test/span.cpp index abf3a5d6..c37e2a66 100644 --- a/test/span.cpp +++ b/test/span.cpp @@ -37,7 +37,7 @@ protected: * to generate undefined behaviour. */ - Span<int, 0>{}; + /* Span<int, 0>{}; */ /* Span<int, 4>{}; */ Span<int, 4>{ &i[0], 4 };
The new default constructor allows to construct a fixed-sized Span via the default constructor of its stored data type. This prevents the construction of empty fixed-sized Spans that cannot hold any data. Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> --- include/libcamera/base/span.h | 5 +++++ include/libcamera/controls.h | 2 +- test/span.cpp | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) -- 2.25.1