| Message ID | 20241001160329.725278-1-pobrn@protonmail.com |
|---|---|
| State | Accepted |
| Commit | e879a86979ab368cd3744bd82fdc7c3013685f0f |
| Headers | show |
| Series |
|
| Related | show |
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@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. Reviewed-by: Kieran Bingham <kieran.bingham@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 > >
> 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@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@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 > > > > >
On Wed, Oct 02, 2024 at 10:53:02PM +0100, Kieran Bingham wrote: > 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@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. std::string_view is essentially a const char * and a size_t. It's like a span, but with an API optimized for usage with strings. I would have sworn it was a C++20 feature, but it looks like I was wrong. We should use it more frequently, for cases where callers have a const char * (possibly mixed with std::string) and the callee needs the length of the string. There an overhead when construction a std::string_view from a const char * as the length needs to be determined, so we shouldn't use it when the callee doesn't need the length of the string. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@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;
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;
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@protonmail.com> --- include/libcamera/internal/yaml_parser.h | 7 ++++--- src/libcamera/yaml_parser.cpp | 11 ++++------- 2 files changed, 8 insertions(+), 10 deletions(-)