| Message ID | 20260113000808.15395-30-laurent.pinchart@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
2026. 01. 13. 1:08 keltezéssel, Laurent Pinchart írta: > The GlobalConfiguration class will need to add nested children to a > ValueNode. Add a new overload to the add() function for this purpose. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/internal/value_node.h | 3 ++ > src/libcamera/value_node.cpp | 56 +++++++++++++++++++++++++ > 2 files changed, 59 insertions(+) > > diff --git a/include/libcamera/internal/value_node.h b/include/libcamera/internal/value_node.h > index c285be4957ed..abb0991fd87e 100644 > --- a/include/libcamera/internal/value_node.h > +++ b/include/libcamera/internal/value_node.h > @@ -8,6 +8,7 @@ > > #pragma once > > +#include <initializer_list> > #include <iterator> > #include <map> > #include <memory> > @@ -237,6 +238,8 @@ public: > > ValueNode *add(std::unique_ptr<ValueNode> child); > ValueNode *add(std::string key, std::unique_ptr<ValueNode> child); > + ValueNode *add(std::initializer_list<std::string_view> path, > + std::unique_ptr<ValueNode> child); > > private: > LIBCAMERA_DISABLE_COPY_AND_MOVE(ValueNode) > diff --git a/src/libcamera/value_node.cpp b/src/libcamera/value_node.cpp > index 50b23284f930..1256adf8361d 100644 > --- a/src/libcamera/value_node.cpp > +++ b/src/libcamera/value_node.cpp > @@ -13,6 +13,8 @@ > #include <string> > #include <vector> > > +#include <libcamera/base/log.h> > +#include <libcamera/base/span.h> > #include <libcamera/base/utils.h> > > /** > @@ -22,6 +24,8 @@ > > namespace libcamera { > > +LOG_DEFINE_CATEGORY(ValueNode) > + > namespace { > > /* Empty static ValueNode as a safe result for invalid operations */ > @@ -539,4 +543,56 @@ ValueNode *ValueNode::add(std::string key, std::unique_ptr<ValueNode> child) > return elem.value.get(); > } > > +/** > + * \brief Add a child node at the given path > + * \param[in] path The path > + * \param[in] child The child node > + * > + * Add the \a child node at the given \a path starting at this node. Missing > + * nodes are created along the path. Nodes along the path must be empty, in > + * which case they are converted to the Type::Dictionary type, be a dictionary, It's a small thing, but I'd put the "in ... type" part in parenthesis because for me it currently disrupts the flow of the list a bit. > + * or be missing. Otherwise, the \a child is discarded and the function returns > + * a nullptr. I think the caveat that already created nodes along `path` are left in place should be mentioned. Maybe even as a `\note`. Or maybe the first non-existent child can be recorded and everything up to that can be removed in a `utils::scope_exit` in case of "failure". But maybe not worth the complexity. > + * > + * Path elements are unique in the context of a parent node. If a child with the > + * same \a key already exist at the end of the path, the \a child is discarded > + * and the function returns a nullptr. > + * > + * \return A pointer to the \a child node if successfully added, nullptr > + * otherwise > + */ > +ValueNode *ValueNode::add(std::initializer_list<std::string_view> path, > + std::unique_ptr<ValueNode> child) I think maybe we could use `std::unique_ptr<ValueNode> &&child` and only actually move the value if the "insertion" will succeed. (Needs the same change in `ValueNode::add()`.) > +{ > + if (!path.size()) > + return NULL; nullptr > + > + ValueNode *node = this; > + > + for (const auto [i, name] : utils::enumerate(path)) { > + auto iter = node->dictionary_.find(name); > + if (iter == node->dictionary_.end()) { > + std::unique_ptr<ValueNode> obj; > + > + if (i < path.size() - 1) > + obj = std::make_unique<ValueNode>(); > + else > + obj = std::move(child); > + > + node = node->add(std::string{ name }, std::move(obj)); > + if (!node) { > + Span<const std::string_view> pathName{ std::data(path), i + 1 }; > + LOG(ValueNode, Error) > + << "Failed to populate '" > + << utils::join(pathName, "/") << "'"; > + return nullptr; > + } > + } else { > + node = iter->second; > + } > + } > + > + return node; > +} > + > } /* namespace libcamera */
On Tue, Jan 13, 2026 at 01:45:22PM +0100, Barnabás Pőcze wrote: > 2026. 01. 13. 1:08 keltezéssel, Laurent Pinchart írta: > > The GlobalConfiguration class will need to add nested children to a > > ValueNode. Add a new overload to the add() function for this purpose. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/internal/value_node.h | 3 ++ > > src/libcamera/value_node.cpp | 56 +++++++++++++++++++++++++ > > 2 files changed, 59 insertions(+) > > > > diff --git a/include/libcamera/internal/value_node.h b/include/libcamera/internal/value_node.h > > index c285be4957ed..abb0991fd87e 100644 > > --- a/include/libcamera/internal/value_node.h > > +++ b/include/libcamera/internal/value_node.h > > @@ -8,6 +8,7 @@ > > > > #pragma once > > > > +#include <initializer_list> > > #include <iterator> > > #include <map> > > #include <memory> > > @@ -237,6 +238,8 @@ public: > > > > ValueNode *add(std::unique_ptr<ValueNode> child); > > ValueNode *add(std::string key, std::unique_ptr<ValueNode> child); > > + ValueNode *add(std::initializer_list<std::string_view> path, > > + std::unique_ptr<ValueNode> child); > > > > private: > > LIBCAMERA_DISABLE_COPY_AND_MOVE(ValueNode) > > diff --git a/src/libcamera/value_node.cpp b/src/libcamera/value_node.cpp > > index 50b23284f930..1256adf8361d 100644 > > --- a/src/libcamera/value_node.cpp > > +++ b/src/libcamera/value_node.cpp > > @@ -13,6 +13,8 @@ > > #include <string> > > #include <vector> > > > > +#include <libcamera/base/log.h> > > +#include <libcamera/base/span.h> > > #include <libcamera/base/utils.h> > > > > /** > > @@ -22,6 +24,8 @@ > > > > namespace libcamera { > > > > +LOG_DEFINE_CATEGORY(ValueNode) > > + > > namespace { > > > > /* Empty static ValueNode as a safe result for invalid operations */ > > @@ -539,4 +543,56 @@ ValueNode *ValueNode::add(std::string key, std::unique_ptr<ValueNode> child) > > return elem.value.get(); > > } > > > > +/** > > + * \brief Add a child node at the given path > > + * \param[in] path The path > > + * \param[in] child The child node > > + * > > + * Add the \a child node at the given \a path starting at this node. Missing > > + * nodes are created along the path. Nodes along the path must be empty, in > > + * which case they are converted to the Type::Dictionary type, be a dictionary, > > It's a small thing, but I'd put the "in ... type" part in parenthesis because for > me it currently disrupts the flow of the list a bit. Good idea. > > + * or be missing. Otherwise, the \a child is discarded and the function returns > > + * a nullptr. > > I think the caveat that already created nodes along `path` are left in place > should be mentioned. Maybe even as a `\note`. > > Or maybe the first non-existent child can be recorded and everything up to that > can be removed in a `utils::scope_exit` in case of "failure". But maybe not worth > the complexity. I'll add a note. > > + * > > + * Path elements are unique in the context of a parent node. If a child with the > > + * same \a key already exist at the end of the path, the \a child is discarded > > + * and the function returns a nullptr. > > + * > > + * \return A pointer to the \a child node if successfully added, nullptr > > + * otherwise > > + */ > > +ValueNode *ValueNode::add(std::initializer_list<std::string_view> path, > > + std::unique_ptr<ValueNode> child) > > I think maybe we could use `std::unique_ptr<ValueNode> &&child` and only actually > move the value if the "insertion" will succeed. (Needs the same change in `ValueNode::add()`.) Hmmm... I think that would be confusing. The caller would need to do ValueNode &node = ... std::unique_ptr<ValueNode> child = ...; node.add("path", std::move(child)); but the move wouldn't be performed in case of failure. Sure, the child would eventually be freed, but at an unexpected point, and the std::move() call would be misleading. > > +{ > > + if (!path.size()) > > + return NULL; > > nullptr Oops. > > + > > + ValueNode *node = this; > > + > > + for (const auto [i, name] : utils::enumerate(path)) { > > + auto iter = node->dictionary_.find(name); > > + if (iter == node->dictionary_.end()) { > > + std::unique_ptr<ValueNode> obj; > > + > > + if (i < path.size() - 1) > > + obj = std::make_unique<ValueNode>(); > > + else > > + obj = std::move(child); > > + > > + node = node->add(std::string{ name }, std::move(obj)); > > + if (!node) { > > + Span<const std::string_view> pathName{ std::data(path), i + 1 }; > > + LOG(ValueNode, Error) > > + << "Failed to populate '" > > + << utils::join(pathName, "/") << "'"; > > + return nullptr; > > + } > > + } else { > > + node = iter->second; > > + } > > + } > > + > > + return node; > > +} > > + > > } /* namespace libcamera */
2026. 01. 13. 14:50 keltezéssel, Laurent Pinchart írta: > On Tue, Jan 13, 2026 at 01:45:22PM +0100, Barnabás Pőcze wrote: >> 2026. 01. 13. 1:08 keltezéssel, Laurent Pinchart írta: >>> The GlobalConfiguration class will need to add nested children to a >>> ValueNode. Add a new overload to the add() function for this purpose. >>> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> include/libcamera/internal/value_node.h | 3 ++ >>> src/libcamera/value_node.cpp | 56 +++++++++++++++++++++++++ >>> 2 files changed, 59 insertions(+) >>> >>> diff --git a/include/libcamera/internal/value_node.h b/include/libcamera/internal/value_node.h >>> index c285be4957ed..abb0991fd87e 100644 >>> --- a/include/libcamera/internal/value_node.h >>> +++ b/include/libcamera/internal/value_node.h >>> @@ -8,6 +8,7 @@ >>> >>> #pragma once >>> >>> +#include <initializer_list> >>> #include <iterator> >>> #include <map> >>> #include <memory> >>> @@ -237,6 +238,8 @@ public: >>> >>> ValueNode *add(std::unique_ptr<ValueNode> child); >>> ValueNode *add(std::string key, std::unique_ptr<ValueNode> child); >>> + ValueNode *add(std::initializer_list<std::string_view> path, >>> + std::unique_ptr<ValueNode> child); >>> >>> private: >>> LIBCAMERA_DISABLE_COPY_AND_MOVE(ValueNode) >>> diff --git a/src/libcamera/value_node.cpp b/src/libcamera/value_node.cpp >>> index 50b23284f930..1256adf8361d 100644 >>> --- a/src/libcamera/value_node.cpp >>> +++ b/src/libcamera/value_node.cpp >>> @@ -13,6 +13,8 @@ >>> #include <string> >>> #include <vector> >>> >>> +#include <libcamera/base/log.h> >>> +#include <libcamera/base/span.h> >>> #include <libcamera/base/utils.h> >>> >>> /** >>> @@ -22,6 +24,8 @@ >>> >>> namespace libcamera { >>> >>> +LOG_DEFINE_CATEGORY(ValueNode) >>> + >>> namespace { >>> >>> /* Empty static ValueNode as a safe result for invalid operations */ >>> @@ -539,4 +543,56 @@ ValueNode *ValueNode::add(std::string key, std::unique_ptr<ValueNode> child) >>> return elem.value.get(); >>> } >>> >>> +/** >>> + * \brief Add a child node at the given path >>> + * \param[in] path The path >>> + * \param[in] child The child node >>> + * >>> + * Add the \a child node at the given \a path starting at this node. Missing >>> + * nodes are created along the path. Nodes along the path must be empty, in >>> + * which case they are converted to the Type::Dictionary type, be a dictionary, >> >> It's a small thing, but I'd put the "in ... type" part in parenthesis because for >> me it currently disrupts the flow of the list a bit. > > Good idea. > >>> + * or be missing. Otherwise, the \a child is discarded and the function returns >>> + * a nullptr. >> >> I think the caveat that already created nodes along `path` are left in place >> should be mentioned. Maybe even as a `\note`. >> >> Or maybe the first non-existent child can be recorded and everything up to that >> can be removed in a `utils::scope_exit` in case of "failure". But maybe not worth >> the complexity. > > I'll add a note. > >>> + * >>> + * Path elements are unique in the context of a parent node. If a child with the >>> + * same \a key already exist at the end of the path, the \a child is discarded >>> + * and the function returns a nullptr. >>> + * >>> + * \return A pointer to the \a child node if successfully added, nullptr >>> + * otherwise >>> + */ >>> +ValueNode *ValueNode::add(std::initializer_list<std::string_view> path, >>> + std::unique_ptr<ValueNode> child) >> >> I think maybe we could use `std::unique_ptr<ValueNode> &&child` and only actually >> move the value if the "insertion" will succeed. (Needs the same change in `ValueNode::add()`.) > > Hmmm... I think that would be confusing. The caller would need to do > > ValueNode &node = ... > std::unique_ptr<ValueNode> child = ...; > > node.add("path", std::move(child)); Well, they can do `node.add("...", std::make_unique<ValueNode>(...))` as well. But then the difference is not really visible. > > but the move wouldn't be performed in case of failure. Sure, the child > would eventually be freed, but at an unexpected point, and the > std::move() call would be misleading. I suppose it depends on one's expectations about `std::move()`. Technically it is a cast, so it doesn't do anything in and of itself. So for example I think you have the same behaviour if e.g. `vec.push_back(std::move(...))` fails. > >>> +{ >>> + if (!path.size()) >>> + return NULL; >> >> nullptr > > Oops. > >>> + >>> + ValueNode *node = this; >>> + >>> + for (const auto [i, name] : utils::enumerate(path)) { >>> + auto iter = node->dictionary_.find(name); >>> + if (iter == node->dictionary_.end()) { >>> + std::unique_ptr<ValueNode> obj; >>> + >>> + if (i < path.size() - 1) >>> + obj = std::make_unique<ValueNode>(); >>> + else >>> + obj = std::move(child); >>> + >>> + node = node->add(std::string{ name }, std::move(obj)); >>> + if (!node) { >>> + Span<const std::string_view> pathName{ std::data(path), i + 1 }; >>> + LOG(ValueNode, Error) >>> + << "Failed to populate '" >>> + << utils::join(pathName, "/") << "'"; >>> + return nullptr; >>> + } >>> + } else { >>> + node = iter->second; >>> + } >>> + } >>> + >>> + return node; >>> +} >>> + >>> } /* namespace libcamera */ >
On Tue, Jan 13, 2026 at 04:09:18PM +0100, Barnabás Pőcze wrote: > 2026. 01. 13. 14:50 keltezéssel, Laurent Pinchart írta: > > On Tue, Jan 13, 2026 at 01:45:22PM +0100, Barnabás Pőcze wrote: > >> 2026. 01. 13. 1:08 keltezéssel, Laurent Pinchart írta: > >>> The GlobalConfiguration class will need to add nested children to a > >>> ValueNode. Add a new overload to the add() function for this purpose. > >>> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> include/libcamera/internal/value_node.h | 3 ++ > >>> src/libcamera/value_node.cpp | 56 +++++++++++++++++++++++++ > >>> 2 files changed, 59 insertions(+) > >>> > >>> diff --git a/include/libcamera/internal/value_node.h b/include/libcamera/internal/value_node.h > >>> index c285be4957ed..abb0991fd87e 100644 > >>> --- a/include/libcamera/internal/value_node.h > >>> +++ b/include/libcamera/internal/value_node.h > >>> @@ -8,6 +8,7 @@ > >>> > >>> #pragma once > >>> > >>> +#include <initializer_list> > >>> #include <iterator> > >>> #include <map> > >>> #include <memory> > >>> @@ -237,6 +238,8 @@ public: > >>> > >>> ValueNode *add(std::unique_ptr<ValueNode> child); > >>> ValueNode *add(std::string key, std::unique_ptr<ValueNode> child); > >>> + ValueNode *add(std::initializer_list<std::string_view> path, > >>> + std::unique_ptr<ValueNode> child); > >>> > >>> private: > >>> LIBCAMERA_DISABLE_COPY_AND_MOVE(ValueNode) > >>> diff --git a/src/libcamera/value_node.cpp b/src/libcamera/value_node.cpp > >>> index 50b23284f930..1256adf8361d 100644 > >>> --- a/src/libcamera/value_node.cpp > >>> +++ b/src/libcamera/value_node.cpp > >>> @@ -13,6 +13,8 @@ > >>> #include <string> > >>> #include <vector> > >>> > >>> +#include <libcamera/base/log.h> > >>> +#include <libcamera/base/span.h> > >>> #include <libcamera/base/utils.h> > >>> > >>> /** > >>> @@ -22,6 +24,8 @@ > >>> > >>> namespace libcamera { > >>> > >>> +LOG_DEFINE_CATEGORY(ValueNode) > >>> + > >>> namespace { > >>> > >>> /* Empty static ValueNode as a safe result for invalid operations */ > >>> @@ -539,4 +543,56 @@ ValueNode *ValueNode::add(std::string key, std::unique_ptr<ValueNode> child) > >>> return elem.value.get(); > >>> } > >>> > >>> +/** > >>> + * \brief Add a child node at the given path > >>> + * \param[in] path The path > >>> + * \param[in] child The child node > >>> + * > >>> + * Add the \a child node at the given \a path starting at this node. Missing > >>> + * nodes are created along the path. Nodes along the path must be empty, in > >>> + * which case they are converted to the Type::Dictionary type, be a dictionary, > >> > >> It's a small thing, but I'd put the "in ... type" part in parenthesis because for > >> me it currently disrupts the flow of the list a bit. > > > > Good idea. > > > >>> + * or be missing. Otherwise, the \a child is discarded and the function returns > >>> + * a nullptr. > >> > >> I think the caveat that already created nodes along `path` are left in place > >> should be mentioned. Maybe even as a `\note`. > >> > >> Or maybe the first non-existent child can be recorded and everything up to that > >> can be removed in a `utils::scope_exit` in case of "failure". But maybe not worth > >> the complexity. > > > > I'll add a note. > > > >>> + * > >>> + * Path elements are unique in the context of a parent node. If a child with the > >>> + * same \a key already exist at the end of the path, the \a child is discarded > >>> + * and the function returns a nullptr. > >>> + * > >>> + * \return A pointer to the \a child node if successfully added, nullptr > >>> + * otherwise > >>> + */ > >>> +ValueNode *ValueNode::add(std::initializer_list<std::string_view> path, > >>> + std::unique_ptr<ValueNode> child) > >> > >> I think maybe we could use `std::unique_ptr<ValueNode> &&child` and only actually > >> move the value if the "insertion" will succeed. (Needs the same change in `ValueNode::add()`.) > > > > Hmmm... I think that would be confusing. The caller would need to do > > > > ValueNode &node = ... > > std::unique_ptr<ValueNode> child = ...; > > > > node.add("path", std::move(child)); > > Well, they can do `node.add("...", std::make_unique<ValueNode>(...))` as well. > But then the difference is not really visible. > > > but the move wouldn't be performed in case of failure. Sure, the child > > would eventually be freed, but at an unexpected point, and the > > std::move() call would be misleading. > > I suppose it depends on one's expectations about `std::move()`. Technically > it is a cast, so it doesn't do anything in and of itself. So for example I > think you have the same behaviour if e.g. `vec.push_back(std::move(...))` fails. You're right, but I find that confusing. Do you think we should pass an rvalue reference to the child as a performance improvement, or because the semantics is better ? > >>> +{ > >>> + if (!path.size()) > >>> + return NULL; > >> > >> nullptr > > > > Oops. > > > >>> + > >>> + ValueNode *node = this; > >>> + > >>> + for (const auto [i, name] : utils::enumerate(path)) { > >>> + auto iter = node->dictionary_.find(name); > >>> + if (iter == node->dictionary_.end()) { > >>> + std::unique_ptr<ValueNode> obj; > >>> + > >>> + if (i < path.size() - 1) > >>> + obj = std::make_unique<ValueNode>(); > >>> + else > >>> + obj = std::move(child); > >>> + > >>> + node = node->add(std::string{ name }, std::move(obj)); > >>> + if (!node) { > >>> + Span<const std::string_view> pathName{ std::data(path), i + 1 }; > >>> + LOG(ValueNode, Error) > >>> + << "Failed to populate '" > >>> + << utils::join(pathName, "/") << "'"; > >>> + return nullptr; > >>> + } > >>> + } else { > >>> + node = iter->second; > >>> + } > >>> + } > >>> + > >>> + return node; > >>> +} > >>> + > >>> } /* namespace libcamera */
2026. 01. 19. 3:00 keltezéssel, Laurent Pinchart írta: > On Tue, Jan 13, 2026 at 04:09:18PM +0100, Barnabás Pőcze wrote: >> 2026. 01. 13. 14:50 keltezéssel, Laurent Pinchart írta: >>> On Tue, Jan 13, 2026 at 01:45:22PM +0100, Barnabás Pőcze wrote: >>>> 2026. 01. 13. 1:08 keltezéssel, Laurent Pinchart írta: >>>>> The GlobalConfiguration class will need to add nested children to a >>>>> ValueNode. Add a new overload to the add() function for this purpose. >>>>> >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>>> --- >>>>> include/libcamera/internal/value_node.h | 3 ++ >>>>> src/libcamera/value_node.cpp | 56 +++++++++++++++++++++++++ >>>>> 2 files changed, 59 insertions(+) >>>>> >>>>> diff --git a/include/libcamera/internal/value_node.h b/include/libcamera/internal/value_node.h >>>>> index c285be4957ed..abb0991fd87e 100644 >>>>> --- a/include/libcamera/internal/value_node.h >>>>> +++ b/include/libcamera/internal/value_node.h >>>>> @@ -8,6 +8,7 @@ >>>>> >>>>> #pragma once >>>>> >>>>> +#include <initializer_list> >>>>> #include <iterator> >>>>> #include <map> >>>>> #include <memory> >>>>> @@ -237,6 +238,8 @@ public: >>>>> >>>>> ValueNode *add(std::unique_ptr<ValueNode> child); >>>>> ValueNode *add(std::string key, std::unique_ptr<ValueNode> child); >>>>> + ValueNode *add(std::initializer_list<std::string_view> path, >>>>> + std::unique_ptr<ValueNode> child); >>>>> >>>>> private: >>>>> LIBCAMERA_DISABLE_COPY_AND_MOVE(ValueNode) >>>>> diff --git a/src/libcamera/value_node.cpp b/src/libcamera/value_node.cpp >>>>> index 50b23284f930..1256adf8361d 100644 >>>>> --- a/src/libcamera/value_node.cpp >>>>> +++ b/src/libcamera/value_node.cpp >>>>> @@ -13,6 +13,8 @@ >>>>> #include <string> >>>>> #include <vector> >>>>> >>>>> +#include <libcamera/base/log.h> >>>>> +#include <libcamera/base/span.h> >>>>> #include <libcamera/base/utils.h> >>>>> >>>>> /** >>>>> @@ -22,6 +24,8 @@ >>>>> >>>>> namespace libcamera { >>>>> >>>>> +LOG_DEFINE_CATEGORY(ValueNode) >>>>> + >>>>> namespace { >>>>> >>>>> /* Empty static ValueNode as a safe result for invalid operations */ >>>>> @@ -539,4 +543,56 @@ ValueNode *ValueNode::add(std::string key, std::unique_ptr<ValueNode> child) >>>>> return elem.value.get(); >>>>> } >>>>> >>>>> +/** >>>>> + * \brief Add a child node at the given path >>>>> + * \param[in] path The path >>>>> + * \param[in] child The child node >>>>> + * >>>>> + * Add the \a child node at the given \a path starting at this node. Missing >>>>> + * nodes are created along the path. Nodes along the path must be empty, in >>>>> + * which case they are converted to the Type::Dictionary type, be a dictionary, >>>> >>>> It's a small thing, but I'd put the "in ... type" part in parenthesis because for >>>> me it currently disrupts the flow of the list a bit. >>> >>> Good idea. >>> >>>>> + * or be missing. Otherwise, the \a child is discarded and the function returns >>>>> + * a nullptr. >>>> >>>> I think the caveat that already created nodes along `path` are left in place >>>> should be mentioned. Maybe even as a `\note`. >>>> >>>> Or maybe the first non-existent child can be recorded and everything up to that >>>> can be removed in a `utils::scope_exit` in case of "failure". But maybe not worth >>>> the complexity. >>> >>> I'll add a note. >>> >>>>> + * >>>>> + * Path elements are unique in the context of a parent node. If a child with the >>>>> + * same \a key already exist at the end of the path, the \a child is discarded >>>>> + * and the function returns a nullptr. >>>>> + * >>>>> + * \return A pointer to the \a child node if successfully added, nullptr >>>>> + * otherwise >>>>> + */ >>>>> +ValueNode *ValueNode::add(std::initializer_list<std::string_view> path, >>>>> + std::unique_ptr<ValueNode> child) >>>> >>>> I think maybe we could use `std::unique_ptr<ValueNode> &&child` and only actually >>>> move the value if the "insertion" will succeed. (Needs the same change in `ValueNode::add()`.) >>> >>> Hmmm... I think that would be confusing. The caller would need to do >>> >>> ValueNode &node = ... >>> std::unique_ptr<ValueNode> child = ...; >>> >>> node.add("path", std::move(child)); >> >> Well, they can do `node.add("...", std::make_unique<ValueNode>(...))` as well. >> But then the difference is not really visible. >> >>> but the move wouldn't be performed in case of failure. Sure, the child >>> would eventually be freed, but at an unexpected point, and the >>> std::move() call would be misleading. >> >> I suppose it depends on one's expectations about `std::move()`. Technically >> it is a cast, so it doesn't do anything in and of itself. So for example I >> think you have the same behaviour if e.g. `vec.push_back(std::move(...))` fails. > > You're right, but I find that confusing. Do you think we should pass an > rvalue reference to the child as a performance improvement, or because > the semantics is better ? I like the semantics better, it is more "transactional": if anything goes wrong, it is as if the function has never been called. But I suppose this is a very minor thing, and this is an internal function, so it can be easily modified if a real use case presents itself. > >>>>> +{ >>>>> + if (!path.size()) >>>>> + return NULL; >>>> >>>> nullptr >>> >>> Oops. >>> >>>>> + >>>>> + ValueNode *node = this; >>>>> + >>>>> + for (const auto [i, name] : utils::enumerate(path)) { >>>>> + auto iter = node->dictionary_.find(name); >>>>> + if (iter == node->dictionary_.end()) { >>>>> + std::unique_ptr<ValueNode> obj; >>>>> + >>>>> + if (i < path.size() - 1) >>>>> + obj = std::make_unique<ValueNode>(); >>>>> + else >>>>> + obj = std::move(child); >>>>> + >>>>> + node = node->add(std::string{ name }, std::move(obj)); >>>>> + if (!node) { >>>>> + Span<const std::string_view> pathName{ std::data(path), i + 1 }; >>>>> + LOG(ValueNode, Error) >>>>> + << "Failed to populate '" >>>>> + << utils::join(pathName, "/") << "'"; >>>>> + return nullptr; >>>>> + } >>>>> + } else { >>>>> + node = iter->second; >>>>> + } >>>>> + } >>>>> + >>>>> + return node; >>>>> +} >>>>> + >>>>> } /* namespace libcamera */ > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/internal/value_node.h b/include/libcamera/internal/value_node.h index c285be4957ed..abb0991fd87e 100644 --- a/include/libcamera/internal/value_node.h +++ b/include/libcamera/internal/value_node.h @@ -8,6 +8,7 @@ #pragma once +#include <initializer_list> #include <iterator> #include <map> #include <memory> @@ -237,6 +238,8 @@ public: ValueNode *add(std::unique_ptr<ValueNode> child); ValueNode *add(std::string key, std::unique_ptr<ValueNode> child); + ValueNode *add(std::initializer_list<std::string_view> path, + std::unique_ptr<ValueNode> child); private: LIBCAMERA_DISABLE_COPY_AND_MOVE(ValueNode) diff --git a/src/libcamera/value_node.cpp b/src/libcamera/value_node.cpp index 50b23284f930..1256adf8361d 100644 --- a/src/libcamera/value_node.cpp +++ b/src/libcamera/value_node.cpp @@ -13,6 +13,8 @@ #include <string> #include <vector> +#include <libcamera/base/log.h> +#include <libcamera/base/span.h> #include <libcamera/base/utils.h> /** @@ -22,6 +24,8 @@ namespace libcamera { +LOG_DEFINE_CATEGORY(ValueNode) + namespace { /* Empty static ValueNode as a safe result for invalid operations */ @@ -539,4 +543,56 @@ ValueNode *ValueNode::add(std::string key, std::unique_ptr<ValueNode> child) return elem.value.get(); } +/** + * \brief Add a child node at the given path + * \param[in] path The path + * \param[in] child The child node + * + * Add the \a child node at the given \a path starting at this node. Missing + * nodes are created along the path. Nodes along the path must be empty, in + * which case they are converted to the Type::Dictionary type, be a dictionary, + * or be missing. Otherwise, the \a child is discarded and the function returns + * a nullptr. + * + * Path elements are unique in the context of a parent node. If a child with the + * same \a key already exist at the end of the path, the \a child is discarded + * and the function returns a nullptr. + * + * \return A pointer to the \a child node if successfully added, nullptr + * otherwise + */ +ValueNode *ValueNode::add(std::initializer_list<std::string_view> path, + std::unique_ptr<ValueNode> child) +{ + if (!path.size()) + return NULL; + + ValueNode *node = this; + + for (const auto [i, name] : utils::enumerate(path)) { + auto iter = node->dictionary_.find(name); + if (iter == node->dictionary_.end()) { + std::unique_ptr<ValueNode> obj; + + if (i < path.size() - 1) + obj = std::make_unique<ValueNode>(); + else + obj = std::move(child); + + node = node->add(std::string{ name }, std::move(obj)); + if (!node) { + Span<const std::string_view> pathName{ std::data(path), i + 1 }; + LOG(ValueNode, Error) + << "Failed to populate '" + << utils::join(pathName, "/") << "'"; + return nullptr; + } + } else { + node = iter->second; + } + } + + return node; +} + } /* namespace libcamera */
The GlobalConfiguration class will need to add nested children to a ValueNode. Add a new overload to the add() function for this purpose. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/internal/value_node.h | 3 ++ src/libcamera/value_node.cpp | 56 +++++++++++++++++++++++++ 2 files changed, 59 insertions(+)