| Message ID | 20260503225315.1272813-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
2026. 05. 04. 0:53 keltezéssel, Laurent Pinchart írta: > This reverts commit e0e54965df015b954d75ebe945221372f2dffb80. > > utils::defopt was removed due to compilation errors with gcc 8 that were > deemed impossible to work around. Now that libcamera has dropped support > for gcc 8, restore the utility. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/base/utils.h | 14 +++++++++ > src/libcamera/base/utils.cpp | 21 ++++++++++++++ > test/utils.cpp | 53 ++++++++++++++++++++++++++++++++++ > 3 files changed, 88 insertions(+) > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > index b33a4c644a87..c962b2a3ba8e 100644 > --- a/include/libcamera/base/utils.h > +++ b/include/libcamera/base/utils.h > @@ -430,6 +430,20 @@ scope_exit(EF) -> scope_exit<EF>; > > #endif /* __DOXYGEN__ */ > > +namespace details { > + > +struct defopt_t { > + template<typename T> > + operator T() const constexpr? Although arguably it probably won't get used in that context a lot. > + { Possibly static_assert(std::is_default_constructible_v<T>); could be added to make the error more clear. > + return T{}; > + } > +}; > + > +} /* namespace details */ > + > +constexpr details::defopt_t defopt; > + > #ifndef __DOXYGEN__ > std::ostream &operator<<(std::ostream &os, const Duration &d); > #endif > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp > index 4ab2bd863e11..7d66547cd2a5 100644 > --- a/src/libcamera/base/utils.cpp > +++ b/src/libcamera/base/utils.cpp > @@ -685,6 +685,27 @@ void ScopeExitActions::release() > actions_.clear(); > } > > +/** > + * \var defopt > + * \brief Constant used with std::optional::value_or() to create a > + * default-constructed object > + * > + * The std::optional<T>::value_or(U &&default_value) function returns the > + * contained value if available, or \a default_value if the std::optional has no > + * value. If the desired default value is a default-constructed T, the obvious > + * option is to call std::optional<T>::value_or(T{}). This approach has two > + * drawbacks: > + * > + * * The \a default_value T{} is constructed even if the std::optional instance > + * has a value, which impacts efficiency. > + * * The T{} default constructor needs to be spelled out explicitly in the > + * value_or() call, leading to long lines if the type is complex. This is not quite true anymore: https://cplusplus.github.io/LWG/issue3886 It's just not used in libcamera because older gcc doesn't have the change. > + * > + * The defopt variable solves these issues by providing a value that can be > + * passed to std::optional<T>::value_or() and get implicitly converted to a > + * default-constructed T. > + */ > + > #ifndef __DOXYGEN__ > std::ostream &operator<<(std::ostream &os, const Duration &d) > { > diff --git a/test/utils.cpp b/test/utils.cpp > index b5ce94e5e912..eba3ed9fbf43 100644 > --- a/test/utils.cpp > +++ b/test/utils.cpp > @@ -178,6 +178,55 @@ protected: > return TestPass; > } > > + int testDefopt() > + { > + static bool defaultConstructed = false; Hmm... why not make this a member variable? Yes, I understand this is a revert, but I feel like small changes like that should be ok Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > + > + struct ValueType { > + ValueType() > + : value_(-1) > + { > + defaultConstructed = true; > + } > + > + ValueType(int value) > + : value_(value) > + { > + } > + > + int value_; > + }; > + > + /* > + * Test that utils::defopt doesn't cause default-construction > + * of a ValueType instance when value_or(utils::defopt) is > + * called on a std::optional that has a value. > + */ > + std::optional<ValueType> opt = ValueType(0); > + ValueType value = opt.value_or(utils::defopt); > + > + if (defaultConstructed || value.value_ != 0) { > + std::cerr << "utils::defopt didn't prevent default construction" > + << std::endl; > + return TestFail; > + } > + > + /* > + * Then test that the ValueType is correctly default-constructed > + * when the std::optional has no value. > + */ > + opt = std::nullopt; > + value = opt.value_or(utils::defopt); > + > + if (!defaultConstructed || value.value_ != -1) { > + std::cerr << "utils::defopt didn't cause default construction" > + << std::endl; > + return TestFail; > + } > + > + return TestPass; > + } > + > int run() > { > /* utils::hex() test. */ > @@ -332,6 +381,10 @@ protected: > if (testDuration() != TestPass) > return TestFail; > > + /* utils::defopt test. */ > + if (testDefopt() != TestPass) > + return TestFail; > + > return TestPass; > } > }; > > base-commit: 183e37362f57ff3ce7493abf0bc6f1b57b931f55
Quoting Barnabás Pőcze (2026-05-04 09:07:07) > 2026. 05. 04. 0:53 keltezéssel, Laurent Pinchart írta: > > This reverts commit e0e54965df015b954d75ebe945221372f2dffb80. > > > > utils::defopt was removed due to compilation errors with gcc 8 that were > > deemed impossible to work around. Now that libcamera has dropped support > > for gcc 8, restore the utility. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/base/utils.h | 14 +++++++++ > > src/libcamera/base/utils.cpp | 21 ++++++++++++++ > > test/utils.cpp | 53 ++++++++++++++++++++++++++++++++++ > > 3 files changed, 88 insertions(+) > > > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > > index b33a4c644a87..c962b2a3ba8e 100644 > > --- a/include/libcamera/base/utils.h > > +++ b/include/libcamera/base/utils.h > > @@ -430,6 +430,20 @@ scope_exit(EF) -> scope_exit<EF>; > > > > #endif /* __DOXYGEN__ */ > > > > +namespace details { > > + > > +struct defopt_t { > > + template<typename T> > > + operator T() const > > constexpr? Although arguably it probably won't get used in that context a lot. > > > > + { > > Possibly > > static_assert(std::is_default_constructible_v<T>); > > could be added to make the error more clear. > > > > + return T{}; > > + } > > +}; > > + > > +} /* namespace details */ > > + > > +constexpr details::defopt_t defopt; > > + > > #ifndef __DOXYGEN__ > > std::ostream &operator<<(std::ostream &os, const Duration &d); > > #endif > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp > > index 4ab2bd863e11..7d66547cd2a5 100644 > > --- a/src/libcamera/base/utils.cpp > > +++ b/src/libcamera/base/utils.cpp > > @@ -685,6 +685,27 @@ void ScopeExitActions::release() > > actions_.clear(); > > } > > > > +/** > > + * \var defopt > > + * \brief Constant used with std::optional::value_or() to create a > > + * default-constructed object > > + * > > + * The std::optional<T>::value_or(U &&default_value) function returns the > > + * contained value if available, or \a default_value if the std::optional has no > > + * value. If the desired default value is a default-constructed T, the obvious > > + * option is to call std::optional<T>::value_or(T{}). This approach has two > > + * drawbacks: > > + * > > + * * The \a default_value T{} is constructed even if the std::optional instance > > + * has a value, which impacts efficiency. > > + * * The T{} default constructor needs to be spelled out explicitly in the > > + * value_or() call, leading to long lines if the type is complex. > > This is not quite true anymore: https://cplusplus.github.io/LWG/issue3886 > It's just not used in libcamera because older gcc doesn't have the change. > > > > + * > > + * The defopt variable solves these issues by providing a value that can be > > + * passed to std::optional<T>::value_or() and get implicitly converted to a > > + * default-constructed T. > > + */ > > + > > #ifndef __DOXYGEN__ > > std::ostream &operator<<(std::ostream &os, const Duration &d) > > { > > diff --git a/test/utils.cpp b/test/utils.cpp > > index b5ce94e5e912..eba3ed9fbf43 100644 > > --- a/test/utils.cpp > > +++ b/test/utils.cpp > > @@ -178,6 +178,55 @@ protected: > > return TestPass; > > } > > > > + int testDefopt() > > + { > > + static bool defaultConstructed = false; > > Hmm... why not make this a member variable? Yes, I understand this is a revert, > but I feel like small changes like that should be ok > > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> It could also be a new patch if preferred rather than a revert, and simply mention that it is based on the revert. Eitherway Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + > > + struct ValueType { > > + ValueType() > > + : value_(-1) > > + { > > + defaultConstructed = true; > > + } > > + > > + ValueType(int value) > > + : value_(value) > > + { > > + } > > + > > + int value_; > > + }; > > + > > + /* > > + * Test that utils::defopt doesn't cause default-construction > > + * of a ValueType instance when value_or(utils::defopt) is > > + * called on a std::optional that has a value. > > + */ > > + std::optional<ValueType> opt = ValueType(0); > > + ValueType value = opt.value_or(utils::defopt); > > + > > + if (defaultConstructed || value.value_ != 0) { > > + std::cerr << "utils::defopt didn't prevent default construction" > > + << std::endl; > > + return TestFail; > > + } > > + > > + /* > > + * Then test that the ValueType is correctly default-constructed > > + * when the std::optional has no value. > > + */ > > + opt = std::nullopt; > > + value = opt.value_or(utils::defopt); > > + > > + if (!defaultConstructed || value.value_ != -1) { > > + std::cerr << "utils::defopt didn't cause default construction" > > + << std::endl; > > + return TestFail; > > + } > > + > > + return TestPass; > > + } > > + > > int run() > > { > > /* utils::hex() test. */ > > @@ -332,6 +381,10 @@ protected: > > if (testDuration() != TestPass) > > return TestFail; > > > > + /* utils::defopt test. */ > > + if (testDefopt() != TestPass) > > + return TestFail; > > + > > return TestPass; > > } > > }; > > > > base-commit: 183e37362f57ff3ce7493abf0bc6f1b57b931f55 >
On Mon, May 04, 2026 at 10:07:07AM +0200, Barnabás Pőcze wrote: > 2026. 05. 04. 0:53 keltezéssel, Laurent Pinchart írta: > > This reverts commit e0e54965df015b954d75ebe945221372f2dffb80. > > > > utils::defopt was removed due to compilation errors with gcc 8 that were > > deemed impossible to work around. Now that libcamera has dropped support > > for gcc 8, restore the utility. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/base/utils.h | 14 +++++++++ > > src/libcamera/base/utils.cpp | 21 ++++++++++++++ > > test/utils.cpp | 53 ++++++++++++++++++++++++++++++++++ > > 3 files changed, 88 insertions(+) > > > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > > index b33a4c644a87..c962b2a3ba8e 100644 > > --- a/include/libcamera/base/utils.h > > +++ b/include/libcamera/base/utils.h > > @@ -430,6 +430,20 @@ scope_exit(EF) -> scope_exit<EF>; > > > > #endif /* __DOXYGEN__ */ > > > > +namespace details { > > + > > +struct defopt_t { > > + template<typename T> > > + operator T() const > > constexpr? Although arguably it probably won't get used in that context a lot. Shouldn't hurt. > > + { > > Possibly > > static_assert(std::is_default_constructible_v<T>); > > could be added to make the error more clear. I don't know what error message the compiler would produce, but I can certainly imagine a satanic chant. I'll add the static assert. > > + return T{}; > > + } > > +}; > > + > > +} /* namespace details */ > > + > > +constexpr details::defopt_t defopt; > > + > > #ifndef __DOXYGEN__ > > std::ostream &operator<<(std::ostream &os, const Duration &d); > > #endif > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp > > index 4ab2bd863e11..7d66547cd2a5 100644 > > --- a/src/libcamera/base/utils.cpp > > +++ b/src/libcamera/base/utils.cpp > > @@ -685,6 +685,27 @@ void ScopeExitActions::release() > > actions_.clear(); > > } > > > > +/** > > + * \var defopt > > + * \brief Constant used with std::optional::value_or() to create a > > + * default-constructed object > > + * > > + * The std::optional<T>::value_or(U &&default_value) function returns the > > + * contained value if available, or \a default_value if the std::optional has no > > + * value. If the desired default value is a default-constructed T, the obvious > > + * option is to call std::optional<T>::value_or(T{}). This approach has two > > + * drawbacks: > > + * > > + * * The \a default_value T{} is constructed even if the std::optional instance > > + * has a value, which impacts efficiency. > > + * * The T{} default constructor needs to be spelled out explicitly in the > > + * value_or() call, leading to long lines if the type is complex. > > This is not quite true anymore: https://cplusplus.github.io/LWG/issue3886 > It's just not used in libcamera because older gcc doesn't have the change. Indeed, thanks for pointing it out. I've tested how various compiler versions react when using {} instead of T{}: - All gcc versions prior to 15 produce an error - gcc 15 compiles the code correctly - So do clang 20 and 21. I'll rework the documentation. A std::optional::value_or() function with no argument that default-constructs a T would have been nice. > > + * > > + * The defopt variable solves these issues by providing a value that can be > > + * passed to std::optional<T>::value_or() and get implicitly converted to a > > + * default-constructed T. > > + */ > > + > > #ifndef __DOXYGEN__ > > std::ostream &operator<<(std::ostream &os, const Duration &d) > > { > > diff --git a/test/utils.cpp b/test/utils.cpp > > index b5ce94e5e912..eba3ed9fbf43 100644 > > --- a/test/utils.cpp > > +++ b/test/utils.cpp > > @@ -178,6 +178,55 @@ protected: > > return TestPass; > > } > > > > + int testDefopt() > > + { > > + static bool defaultConstructed = false; > > Hmm... why not make this a member variable? Yes, I understand this is a revert, > but I feel like small changes like that should be ok I'll change it. > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > + > > + struct ValueType { > > + ValueType() > > + : value_(-1) > > + { > > + defaultConstructed = true; > > + } > > + > > + ValueType(int value) > > + : value_(value) > > + { > > + } > > + > > + int value_; > > + }; > > + > > + /* > > + * Test that utils::defopt doesn't cause default-construction > > + * of a ValueType instance when value_or(utils::defopt) is > > + * called on a std::optional that has a value. > > + */ > > + std::optional<ValueType> opt = ValueType(0); > > + ValueType value = opt.value_or(utils::defopt); > > + > > + if (defaultConstructed || value.value_ != 0) { > > + std::cerr << "utils::defopt didn't prevent default construction" > > + << std::endl; > > + return TestFail; > > + } > > + > > + /* > > + * Then test that the ValueType is correctly default-constructed > > + * when the std::optional has no value. > > + */ > > + opt = std::nullopt; > > + value = opt.value_or(utils::defopt); > > + > > + if (!defaultConstructed || value.value_ != -1) { > > + std::cerr << "utils::defopt didn't cause default construction" > > + << std::endl; > > + return TestFail; > > + } > > + > > + return TestPass; > > + } > > + > > int run() > > { > > /* utils::hex() test. */ > > @@ -332,6 +381,10 @@ protected: > > if (testDuration() != TestPass) > > return TestFail; > > > > + /* utils::defopt test. */ > > + if (testDefopt() != TestPass) > > + return TestFail; > > + > > return TestPass; > > } > > }; > > > > base-commit: 183e37362f57ff3ce7493abf0bc6f1b57b931f55
On Mon, May 04, 2026 at 04:29:51PM +0300, Laurent Pinchart wrote: > On Mon, May 04, 2026 at 10:07:07AM +0200, Barnabás Pőcze wrote: > > 2026. 05. 04. 0:53 keltezéssel, Laurent Pinchart írta: > > > This reverts commit e0e54965df015b954d75ebe945221372f2dffb80. > > > > > > utils::defopt was removed due to compilation errors with gcc 8 that were > > > deemed impossible to work around. Now that libcamera has dropped support > > > for gcc 8, restore the utility. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > include/libcamera/base/utils.h | 14 +++++++++ > > > src/libcamera/base/utils.cpp | 21 ++++++++++++++ > > > test/utils.cpp | 53 ++++++++++++++++++++++++++++++++++ > > > 3 files changed, 88 insertions(+) > > > > > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h > > > index b33a4c644a87..c962b2a3ba8e 100644 > > > --- a/include/libcamera/base/utils.h > > > +++ b/include/libcamera/base/utils.h > > > @@ -430,6 +430,20 @@ scope_exit(EF) -> scope_exit<EF>; > > > > > > #endif /* __DOXYGEN__ */ > > > > > > +namespace details { > > > + > > > +struct defopt_t { > > > + template<typename T> > > > + operator T() const > > > > constexpr? Although arguably it probably won't get used in that context a lot. > > Shouldn't hurt. > > > > + { > > > > Possibly > > > > static_assert(std::is_default_constructible_v<T>); > > > > could be added to make the error more clear. > > I don't know what error message the compiler would produce, but I can > certainly imagine a satanic chant. I'll add the static assert. > > > > + return T{}; > > > + } > > > +}; > > > + > > > +} /* namespace details */ > > > + > > > +constexpr details::defopt_t defopt; > > > + > > > #ifndef __DOXYGEN__ > > > std::ostream &operator<<(std::ostream &os, const Duration &d); > > > #endif > > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp > > > index 4ab2bd863e11..7d66547cd2a5 100644 > > > --- a/src/libcamera/base/utils.cpp > > > +++ b/src/libcamera/base/utils.cpp > > > @@ -685,6 +685,27 @@ void ScopeExitActions::release() > > > actions_.clear(); > > > } > > > > > > +/** > > > + * \var defopt > > > + * \brief Constant used with std::optional::value_or() to create a > > > + * default-constructed object > > > + * > > > + * The std::optional<T>::value_or(U &&default_value) function returns the > > > + * contained value if available, or \a default_value if the std::optional has no > > > + * value. If the desired default value is a default-constructed T, the obvious > > > + * option is to call std::optional<T>::value_or(T{}). This approach has two > > > + * drawbacks: > > > + * > > > + * * The \a default_value T{} is constructed even if the std::optional instance > > > + * has a value, which impacts efficiency. > > > + * * The T{} default constructor needs to be spelled out explicitly in the > > > + * value_or() call, leading to long lines if the type is complex. > > > > This is not quite true anymore: https://cplusplus.github.io/LWG/issue3886 > > It's just not used in libcamera because older gcc doesn't have the change. > > Indeed, thanks for pointing it out. I've tested how various compiler > versions react when using {} instead of T{}: > > - All gcc versions prior to 15 produce an error > - gcc 15 compiles the code correctly > - So do clang 20 and 21. > > I'll rework the documentation. > > A std::optional::value_or() function with no argument that > default-constructs a T would have been nice. > > > > + * > > > + * The defopt variable solves these issues by providing a value that can be > > > + * passed to std::optional<T>::value_or() and get implicitly converted to a > > > + * default-constructed T. > > > + */ > > > + > > > #ifndef __DOXYGEN__ > > > std::ostream &operator<<(std::ostream &os, const Duration &d) > > > { > > > diff --git a/test/utils.cpp b/test/utils.cpp > > > index b5ce94e5e912..eba3ed9fbf43 100644 > > > --- a/test/utils.cpp > > > +++ b/test/utils.cpp > > > @@ -178,6 +178,55 @@ protected: > > > return TestPass; > > > } > > > > > > + int testDefopt() > > > + { > > > + static bool defaultConstructed = false; > > > > Hmm... why not make this a member variable? Yes, I understand this is a revert, > > but I feel like small changes like that should be ok > > I'll change it. I spoke too fast on this one. I can't access a non-static member of the UtilsTest class from the nested ValueType. > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > > + > > > + struct ValueType { > > > + ValueType() > > > + : value_(-1) > > > + { > > > + defaultConstructed = true; > > > + } > > > + > > > + ValueType(int value) > > > + : value_(value) > > > + { > > > + } > > > + > > > + int value_; > > > + }; > > > + > > > + /* > > > + * Test that utils::defopt doesn't cause default-construction > > > + * of a ValueType instance when value_or(utils::defopt) is > > > + * called on a std::optional that has a value. > > > + */ > > > + std::optional<ValueType> opt = ValueType(0); > > > + ValueType value = opt.value_or(utils::defopt); > > > + > > > + if (defaultConstructed || value.value_ != 0) { > > > + std::cerr << "utils::defopt didn't prevent default construction" > > > + << std::endl; > > > + return TestFail; > > > + } > > > + > > > + /* > > > + * Then test that the ValueType is correctly default-constructed > > > + * when the std::optional has no value. > > > + */ > > > + opt = std::nullopt; > > > + value = opt.value_or(utils::defopt); > > > + > > > + if (!defaultConstructed || value.value_ != -1) { > > > + std::cerr << "utils::defopt didn't cause default construction" > > > + << std::endl; > > > + return TestFail; > > > + } > > > + > > > + return TestPass; > > > + } > > > + > > > int run() > > > { > > > /* utils::hex() test. */ > > > @@ -332,6 +381,10 @@ protected: > > > if (testDuration() != TestPass) > > > return TestFail; > > > > > > + /* utils::defopt test. */ > > > + if (testDefopt() != TestPass) > > > + return TestFail; > > > + > > > return TestPass; > > > } > > > }; > > > > > > base-commit: 183e37362f57ff3ce7493abf0bc6f1b57b931f55
2026. 05. 04. 15:39 keltezéssel, Laurent Pinchart írta: > On Mon, May 04, 2026 at 04:29:51PM +0300, Laurent Pinchart wrote: >> On Mon, May 04, 2026 at 10:07:07AM +0200, Barnabás Pőcze wrote: >>> 2026. 05. 04. 0:53 keltezéssel, Laurent Pinchart írta: >>>> This reverts commit e0e54965df015b954d75ebe945221372f2dffb80. >>>> >>>> utils::defopt was removed due to compilation errors with gcc 8 that were >>>> deemed impossible to work around. Now that libcamera has dropped support >>>> for gcc 8, restore the utility. >>>> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> --- >>>> include/libcamera/base/utils.h | 14 +++++++++ >>>> src/libcamera/base/utils.cpp | 21 ++++++++++++++ >>>> test/utils.cpp | 53 ++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 88 insertions(+) >>>> >>>> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h >>>> index b33a4c644a87..c962b2a3ba8e 100644 >>>> --- a/include/libcamera/base/utils.h >>>> +++ b/include/libcamera/base/utils.h >>>> @@ -430,6 +430,20 @@ scope_exit(EF) -> scope_exit<EF>; >>>> >>>> #endif /* __DOXYGEN__ */ >>>> >>>> +namespace details { >>>> + >>>> +struct defopt_t { >>>> + template<typename T> >>>> + operator T() const >>> >>> constexpr? Although arguably it probably won't get used in that context a lot. >> >> Shouldn't hurt. >> >>>> + { >>> >>> Possibly >>> >>> static_assert(std::is_default_constructible_v<T>); >>> >>> could be added to make the error more clear. >> >> I don't know what error message the compiler would produce, but I can >> certainly imagine a satanic chant. I'll add the static assert. >> >>>> + return T{}; >>>> + } >>>> +}; >>>> + >>>> +} /* namespace details */ >>>> + >>>> +constexpr details::defopt_t defopt; >>>> + >>>> #ifndef __DOXYGEN__ >>>> std::ostream &operator<<(std::ostream &os, const Duration &d); >>>> #endif >>>> diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp >>>> index 4ab2bd863e11..7d66547cd2a5 100644 >>>> --- a/src/libcamera/base/utils.cpp >>>> +++ b/src/libcamera/base/utils.cpp >>>> @@ -685,6 +685,27 @@ void ScopeExitActions::release() >>>> actions_.clear(); >>>> } >>>> >>>> +/** >>>> + * \var defopt >>>> + * \brief Constant used with std::optional::value_or() to create a >>>> + * default-constructed object >>>> + * >>>> + * The std::optional<T>::value_or(U &&default_value) function returns the >>>> + * contained value if available, or \a default_value if the std::optional has no >>>> + * value. If the desired default value is a default-constructed T, the obvious >>>> + * option is to call std::optional<T>::value_or(T{}). This approach has two >>>> + * drawbacks: >>>> + * >>>> + * * The \a default_value T{} is constructed even if the std::optional instance >>>> + * has a value, which impacts efficiency. >>>> + * * The T{} default constructor needs to be spelled out explicitly in the >>>> + * value_or() call, leading to long lines if the type is complex. >>> >>> This is not quite true anymore: https://cplusplus.github.io/LWG/issue3886 >>> It's just not used in libcamera because older gcc doesn't have the change. >> >> Indeed, thanks for pointing it out. I've tested how various compiler >> versions react when using {} instead of T{}: >> >> - All gcc versions prior to 15 produce an error >> - gcc 15 compiles the code correctly >> - So do clang 20 and 21. >> >> I'll rework the documentation. >> >> A std::optional::value_or() function with no argument that >> default-constructs a T would have been nice. >> >>>> + * >>>> + * The defopt variable solves these issues by providing a value that can be >>>> + * passed to std::optional<T>::value_or() and get implicitly converted to a >>>> + * default-constructed T. >>>> + */ >>>> + >>>> #ifndef __DOXYGEN__ >>>> std::ostream &operator<<(std::ostream &os, const Duration &d) >>>> { >>>> diff --git a/test/utils.cpp b/test/utils.cpp >>>> index b5ce94e5e912..eba3ed9fbf43 100644 >>>> --- a/test/utils.cpp >>>> +++ b/test/utils.cpp >>>> @@ -178,6 +178,55 @@ protected: >>>> return TestPass; >>>> } >>>> >>>> + int testDefopt() >>>> + { >>>> + static bool defaultConstructed = false; >>> >>> Hmm... why not make this a member variable? Yes, I understand this is a revert, >>> but I feel like small changes like that should be ok >> >> I'll change it. > > I spoke too fast on this one. I can't access a non-static member of the > UtilsTest class from the nested ValueType. Ahh, I meant making it a member of `ValueType`. It's fine anyway. > >>> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>> >>>> + >>>> + struct ValueType { >>>> + ValueType() >>>> + : value_(-1) >>>> + { >>>> + defaultConstructed = true; >>>> + } >>>> + >>>> + ValueType(int value) >>>> + : value_(value) >>>> + { >>>> + } >>>> + >>>> + int value_; >>>> + }; >>>> + >>>> + /* >>>> + * Test that utils::defopt doesn't cause default-construction >>>> + * of a ValueType instance when value_or(utils::defopt) is >>>> + * called on a std::optional that has a value. >>>> + */ >>>> + std::optional<ValueType> opt = ValueType(0); >>>> + ValueType value = opt.value_or(utils::defopt); >>>> + >>>> + if (defaultConstructed || value.value_ != 0) { >>>> + std::cerr << "utils::defopt didn't prevent default construction" >>>> + << std::endl; >>>> + return TestFail; >>>> + } >>>> + >>>> + /* >>>> + * Then test that the ValueType is correctly default-constructed >>>> + * when the std::optional has no value. >>>> + */ >>>> + opt = std::nullopt; >>>> + value = opt.value_or(utils::defopt); >>>> + >>>> + if (!defaultConstructed || value.value_ != -1) { >>>> + std::cerr << "utils::defopt didn't cause default construction" >>>> + << std::endl; >>>> + return TestFail; >>>> + } >>>> + >>>> + return TestPass; >>>> + } >>>> + >>>> int run() >>>> { >>>> /* utils::hex() test. */ >>>> @@ -332,6 +381,10 @@ protected: >>>> if (testDuration() != TestPass) >>>> return TestFail; >>>> >>>> + /* utils::defopt test. */ >>>> + if (testDefopt() != TestPass) >>>> + return TestFail; >>>> + >>>> return TestPass; >>>> } >>>> }; >>>> >>>> base-commit: 183e37362f57ff3ce7493abf0bc6f1b57b931f55 >
diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h index b33a4c644a87..c962b2a3ba8e 100644 --- a/include/libcamera/base/utils.h +++ b/include/libcamera/base/utils.h @@ -430,6 +430,20 @@ scope_exit(EF) -> scope_exit<EF>; #endif /* __DOXYGEN__ */ +namespace details { + +struct defopt_t { + template<typename T> + operator T() const + { + return T{}; + } +}; + +} /* namespace details */ + +constexpr details::defopt_t defopt; + #ifndef __DOXYGEN__ std::ostream &operator<<(std::ostream &os, const Duration &d); #endif diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp index 4ab2bd863e11..7d66547cd2a5 100644 --- a/src/libcamera/base/utils.cpp +++ b/src/libcamera/base/utils.cpp @@ -685,6 +685,27 @@ void ScopeExitActions::release() actions_.clear(); } +/** + * \var defopt + * \brief Constant used with std::optional::value_or() to create a + * default-constructed object + * + * The std::optional<T>::value_or(U &&default_value) function returns the + * contained value if available, or \a default_value if the std::optional has no + * value. If the desired default value is a default-constructed T, the obvious + * option is to call std::optional<T>::value_or(T{}). This approach has two + * drawbacks: + * + * * The \a default_value T{} is constructed even if the std::optional instance + * has a value, which impacts efficiency. + * * The T{} default constructor needs to be spelled out explicitly in the + * value_or() call, leading to long lines if the type is complex. + * + * The defopt variable solves these issues by providing a value that can be + * passed to std::optional<T>::value_or() and get implicitly converted to a + * default-constructed T. + */ + #ifndef __DOXYGEN__ std::ostream &operator<<(std::ostream &os, const Duration &d) { diff --git a/test/utils.cpp b/test/utils.cpp index b5ce94e5e912..eba3ed9fbf43 100644 --- a/test/utils.cpp +++ b/test/utils.cpp @@ -178,6 +178,55 @@ protected: return TestPass; } + int testDefopt() + { + static bool defaultConstructed = false; + + struct ValueType { + ValueType() + : value_(-1) + { + defaultConstructed = true; + } + + ValueType(int value) + : value_(value) + { + } + + int value_; + }; + + /* + * Test that utils::defopt doesn't cause default-construction + * of a ValueType instance when value_or(utils::defopt) is + * called on a std::optional that has a value. + */ + std::optional<ValueType> opt = ValueType(0); + ValueType value = opt.value_or(utils::defopt); + + if (defaultConstructed || value.value_ != 0) { + std::cerr << "utils::defopt didn't prevent default construction" + << std::endl; + return TestFail; + } + + /* + * Then test that the ValueType is correctly default-constructed + * when the std::optional has no value. + */ + opt = std::nullopt; + value = opt.value_or(utils::defopt); + + if (!defaultConstructed || value.value_ != -1) { + std::cerr << "utils::defopt didn't cause default construction" + << std::endl; + return TestFail; + } + + return TestPass; + } + int run() { /* utils::hex() test. */ @@ -332,6 +381,10 @@ protected: if (testDuration() != TestPass) return TestFail; + /* utils::defopt test. */ + if (testDefopt() != TestPass) + return TestFail; + return TestPass; } };
This reverts commit e0e54965df015b954d75ebe945221372f2dffb80. utils::defopt was removed due to compilation errors with gcc 8 that were deemed impossible to work around. Now that libcamera has dropped support for gcc 8, restore the utility. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/base/utils.h | 14 +++++++++ src/libcamera/base/utils.cpp | 21 ++++++++++++++ test/utils.cpp | 53 ++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+) base-commit: 183e37362f57ff3ce7493abf0bc6f1b57b931f55