[libcamera-devel,v2] libcamera: device_enumerator_udev: Use std::string_view

18538 diff mbox series

In `udevNotify()`, constructing an std::string from the device's
associated action is unnecessary as it is only compared against
static strings, and for that purpose an std::string_view works
just as well, while being cheaper to construct.

In the same vein, an std::string_view can be used to
store the device's devnode initially, and the string
construction can be deferred until it is needed.

Furthermore, previously `udev_device_get_devnode()` was
called twice. The extra call is now removed.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 src/libcamera/device_enumerator_udev.cpp | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

--
2.40.0

Hi Barnabás,

Thank you for the patch.

On Tue, Apr 18, 2023 at 04:26:04PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> In `udevNotify()`, constructing an std::string from the device's
> associated action is unnecessary as it is only compared against
> static strings, and for that purpose an std::string_view works
> just as well, while being cheaper to construct.
> 
> In the same vein, an std::string_view can be used to
> store the device's devnode initially, and the string
> construction can be deferred until it is needed.
> 
> Furthermore, previously `udev_device_get_devnode()` was
> called twice. The extra call is now removed.
> 
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/libcamera/device_enumerator_udev.cpp | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> index a63cd360..0abc1248 100644
> --- a/src/libcamera/device_enumerator_udev.cpp
> +++ b/src/libcamera/device_enumerator_udev.cpp
> @@ -13,6 +13,7 @@
>  #include <list>
>  #include <map>
>  #include <string.h>
> +#include <string_view>
>  #include <sys/ioctl.h>
>  #include <sys/sysmacros.h>
>  #include <unistd.h>
> @@ -331,18 +332,18 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum)
>  void DeviceEnumeratorUdev::udevNotify()
>  {
>  	struct udev_device *dev = udev_monitor_receive_device(monitor_);
> -	std::string action(udev_device_get_action(dev));
> -	std::string deviceNode(udev_device_get_devnode(dev));
> +	std::string_view action(udev_device_get_action(dev));
> +	std::string_view deviceNode(udev_device_get_devnode(dev));
> 
>  	LOG(DeviceEnumerator, Debug)
> -		<< action << " device " << udev_device_get_devnode(dev);
> +		<< action << " device " << deviceNode;
> 
>  	if (action == "add") {
>  		addUdevDevice(dev);
>  	} else if (action == "remove") {
>  		const char *subsystem = udev_device_get_subsystem(dev);
>  		if (subsystem && !strcmp(subsystem, "media"))
> -			removeDevice(deviceNode);
> +			removeDevice(std::string(deviceNode));
>  	}
> 
>  	udev_device_unref(dev);

Hi Barnabás

On Tue, Apr 18, 2023 at 04:26:04PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> In `udevNotify()`, constructing an std::string from the device's
> associated action is unnecessary as it is only compared against
> static strings, and for that purpose an std::string_view works
> just as well, while being cheaper to construct.
>
> In the same vein, an std::string_view can be used to
> store the device's devnode initially, and the string
> construction can be deferred until it is needed.
>
> Furthermore, previously `udev_device_get_devnode()` was
> called twice. The extra call is now removed.
>
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>  src/libcamera/device_enumerator_udev.cpp | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> index a63cd360..0abc1248 100644
> --- a/src/libcamera/device_enumerator_udev.cpp
> +++ b/src/libcamera/device_enumerator_udev.cpp
> @@ -13,6 +13,7 @@
>  #include <list>
>  #include <map>
>  #include <string.h>
> +#include <string_view>
>  #include <sys/ioctl.h>
>  #include <sys/sysmacros.h>
>  #include <unistd.h>
> @@ -331,18 +332,18 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum)
>  void DeviceEnumeratorUdev::udevNotify()
>  {
>  	struct udev_device *dev = udev_monitor_receive_device(monitor_);
> -	std::string action(udev_device_get_action(dev));
> -	std::string deviceNode(udev_device_get_devnode(dev));
> +	std::string_view action(udev_device_get_action(dev));
> +	std::string_view deviceNode(udev_device_get_devnode(dev));
>
>  	LOG(DeviceEnumerator, Debug)
> -		<< action << " device " << udev_device_get_devnode(dev);
> +		<< action << " device " << deviceNode;
>
>  	if (action == "add") {
>  		addUdevDevice(dev);
>  	} else if (action == "remove") {
>  		const char *subsystem = udev_device_get_subsystem(dev);
>  		if (subsystem && !strcmp(subsystem, "media"))
> -			removeDevice(deviceNode);
> +			removeDevice(std::string(deviceNode));

Not sure what the actual gain actually is, if we have to construct a
string here, but ok...

Wrapping in a string_view the values returned by
udev_device_get_devnode() and udev_device_get_action() seems safe as
according to `man 3 udev_device_get_devnode`:

        return a pointer to a constant string that describes the requested
        property. The lifetime of this string is bound to the device it was
        requested on

so

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>



>  	}
>
>  	udev_device_unref(dev);
> --
> 2.40.0
>
>