[libcamera-devel] [PATCH 3/5] provide a default fixed-sized Span constructor
Jacopo Mondi
jacopo at jmondi.org
Sat Apr 2 11:46:14 CEST 2022
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.
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
> >>
More information about the libcamera-devel
mailing list