[PATCH v1] libcamera: yaml_parser: Take string keys in `std::string_view`

[Barnabás Pőcze]

> Quoting Barnabás Pőcze (2024-10-01 17:03:32)
> > In many cases a static string literal is used as key. Thus
> > having the argument type be `const std::string&` is suboptimal
> > since an `std::string` object needs to be constructed before
> > the call.
> >
> > C++17 introduced `std::string_view`, using which the call
> > can be done with less overhead, as the `std::string_view`
> > is non-owning and may be passed in registers entirely.
> >
> > So make `YamlObject::{contains,operator[]}` take the string keys
> > in `std::string_view`s.
> >
> > Unfortunately, that is not sufficient yet, because `std::map::find()`
> > takes an reference to `const key_type`, which would be `const std::string&`
> > in the case of `YamlParser`. However, with a transparent comparator
> > such as `std::less<>` `std::map::find()` is able to accept any
> > object as the argument, and it forwards it to the comparator.
> >
> > So make `YamlParser::dictionary_` use `std::less<>` as the comparator
> > to enable the use of `std::map::find()` with any type of argument.
> >
> > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> 
> This passes the CI and unit tests at
> https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1281981
> which includes tests covering the yaml-parser.
> 
> I don't think I would have spotted this ... but it looks and sounds
> reasonable to me ...
> 
> Makes me wonder if sometimes we should just use a raw 'const char *'
> (</me ducks>) ;-) but I think this gives us that with type/length safety
> on top so seems like a win to me.

I think `const std::string&` is generally not the ideal choice for the type of a
function argument. I believe `std::string_view` is preferable, even `const char *`,
when NUL termination is needed because one wants to interface with such APIs.
If both work, then `std::string_view` has the advantage of being more flexible
while avoiding the need to call `strlen()` left and right.


Regards,
Barnabás Pőcze


> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> > ---
> >  include/libcamera/internal/yaml_parser.h |  7 ++++---
> >  src/libcamera/yaml_parser.cpp            | 11 ++++-------
> >  2 files changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/libcamera/internal/yaml_parser.h b/include/libcamera/internal/yaml_parser.h
> > index 6211ff4a..de452844 100644
> > --- a/include/libcamera/internal/yaml_parser.h
> > +++ b/include/libcamera/internal/yaml_parser.h
> > @@ -12,6 +12,7 @@
> >  #include <optional>
> >  #include <stdint.h>
> >  #include <string>
> > +#include <string_view>
> >  #include <vector>
> >
> >  #include <libcamera/base/class.h>
> > @@ -206,8 +207,8 @@ public:
> >
> >         const YamlObject &operator[](std::size_t index) const;
> >
> > -       bool contains(const std::string &key) const;
> > -       const YamlObject &operator[](const std::string &key) const;
> > +       bool contains(std::string_view key) const;
> > +       const YamlObject &operator[](std::string_view key) const;
> >
> >  private:
> >         LIBCAMERA_DISABLE_COPY_AND_MOVE(YamlObject)
> > @@ -232,7 +233,7 @@ private:
> >
> >         std::string value_;
> >         Container list_;
> > -       std::map<std::string, YamlObject *> dictionary_;
> > +       std::map<std::string, YamlObject *, std::less<>> dictionary_;
> >  };
> >
> >  class YamlParser final
> > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> > index 4784f2dc..7c0b341a 100644
> > --- a/src/libcamera/yaml_parser.cpp
> > +++ b/src/libcamera/yaml_parser.cpp
> > @@ -481,16 +481,13 @@ const YamlObject &YamlObject::operator[](std::size_t index) const
> >   *
> >   * \return True if an element exists, false otherwise
> >   */
> > -bool YamlObject::contains(const std::string &key) const
> > +bool YamlObject::contains(std::string_view key) const
> >  {
> > -       if (dictionary_.find(std::ref(key)) == dictionary_.end())
> > -               return false;
> > -
> > -       return true;
> > +       return dictionary_.find(key) != dictionary_.end();
> >  }
> >
> >  /**
> > - * \fn YamlObject::operator[](const std::string &key) const
> > + * \fn YamlObject::operator[](std::string_view key) const
> >   * \brief Retrieve a member by name from the dictionary
> >   *
> >   * This function retrieve a member of a YamlObject by name. Only YamlObject
> > @@ -500,7 +497,7 @@ bool YamlObject::contains(const std::string &key) const
> >   *
> >   * \return The YamlObject corresponding to the \a key member
> >   */
> > -const YamlObject &YamlObject::operator[](const std::string &key) const
> > +const YamlObject &YamlObject::operator[](std::string_view key) const
> >  {
> >         if (type_ != Type::Dictionary)
> >                 return empty;
> > --
> > 2.46.2
> >
> >
>