[libcamera-devel] [PATCH 3/5] provide a default fixed-sized Span constructor
Christian Rauch
Rauch.Christian at gmx.de
Sat Apr 2 13:28:05 CEST 2022
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 at 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
>>>>
More information about the libcamera-devel
mailing list