[libcamera-devel] libcamera: v4l2_device: Return const std::string

287 diff mbox series

Make getter functions of V4L2Device return std::string as the rest of
the library does.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/include/v4l2_device.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--
2.20.1

Hi Jacopo,

On Mon, Jan 21, 2019 at 10:20:20AM +0100, Jacopo Mondi wrote:
> Make getter functions of V4L2Device return std::string as the rest of
> the library does.

This is something that has been discussed during the review of Kieran's
original patch I believe. The question was whether it would be more
efficient to use const char * or a an std::string, and the answer to
that question depends on the expected usage of those methods. If all
users need an std::string, then I agree that the methods below can as
well construct one. On the other hand, if some users need a const char
*, it is a bit pointless to convert from const char * to std::string and
then back to const char * using c_str(). This is why we have opted for
const char * here.

This is a generic question that we should ask ourselves through the
library. Anyone with free time to analyze the problem ? :-)

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/include/v4l2_device.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> index 474c05b..5fe3d82 100644
> --- a/src/libcamera/include/v4l2_device.h
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -45,9 +45,9 @@ public:
>  	bool isOpen() const;
>  	void close();
> 
> -	const char *driverName() const { return caps_.driver(); }
> -	const char *deviceName() const { return caps_.card(); }
> -	const char *busName() const { return caps_.bus_info(); }
> +	const std::string driverName() const { return std::string(caps_.driver()); }
> +	const std::string deviceName() const { return std::string(caps_.card()); }
> +	const std::string busName() const { return std::string(caps_.bus_info()); }

If we wanted to return an std::string, we should drop the const as the
string is returned by value.

> 
>  private:
>  	std::string devnode_;

Hi Jacopo,

On 21/01/2019 09:26, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Mon, Jan 21, 2019 at 10:20:20AM +0100, Jacopo Mondi wrote:
>> Make getter functions of V4L2Device return std::string as the rest of
>> the library does.
>
> This is something that has been discussed during the review of Kieran's
> original patch I believe. The question was whether it would be more
> efficient to use const char * or a an std::string, and the answer to
> that question depends on the expected usage of those methods. If all
> users need an std::string, then I agree that the methods below can as
> well construct one. On the other hand, if some users need a const char
> *, it is a bit pointless to convert from const char * to std::string and
> then back to const char * using c_str(). This is why we have opted for
> const char * here.


And because the compiler can automatically convert from a const char *
to a std::string using the string constructors ... so you can still use
these methods anywhere you would use a std::string...




> This is a generic question that we should ask ourselves through the
> library. Anyone with free time to analyze the problem ? :-)
> 
>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>  src/libcamera/include/v4l2_device.h | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
>> index 474c05b..5fe3d82 100644
>> --- a/src/libcamera/include/v4l2_device.h
>> +++ b/src/libcamera/include/v4l2_device.h
>> @@ -45,9 +45,9 @@ public:
>>  	bool isOpen() const;
>>  	void close();
>>
>> -	const char *driverName() const { return caps_.driver(); }
>> -	const char *deviceName() const { return caps_.card(); }
>> -	const char *busName() const { return caps_.bus_info(); }
>> +	const std::string driverName() const { return std::string(caps_.driver()); }
>> +	const std::string deviceName() const { return std::string(caps_.card()); }
>> +	const std::string busName() const { return std::string(caps_.bus_info()); }
> 
> If we wanted to return an std::string, we should drop the const as the
> string is returned by value.
> 
>>
>>  private:
>>  	std::string devnode_;
>

Hi Kieran,

On Mon, Jan 21, 2019 at 09:34:42AM +0000, Kieran Bingham wrote:
> On 21/01/2019 09:26, Laurent Pinchart wrote:
> > On Mon, Jan 21, 2019 at 10:20:20AM +0100, Jacopo Mondi wrote:
> >> Make getter functions of V4L2Device return std::string as the rest of
> >> the library does.
> >
> > This is something that has been discussed during the review of Kieran's
> > original patch I believe. The question was whether it would be more
> > efficient to use const char * or a an std::string, and the answer to
> > that question depends on the expected usage of those methods. If all
> > users need an std::string, then I agree that the methods below can as
> > well construct one. On the other hand, if some users need a const char
> > *, it is a bit pointless to convert from const char * to std::string and
> > then back to const char * using c_str(). This is why we have opted for
> > const char * here.
> 
> And because the compiler can automatically convert from a const char *
> to a std::string using the string constructors ... so you can still use
> these methods anywhere you would use a std::string...

Note that in that case the std::string constructor will be inlined,
which can be an issue if the function returning const char * is called
from a very large number of places. In this specific case the functions
are themselves inlined anyway, so it would make very little difference.

> > This is a generic question that we should ask ourselves through the
> > library. Anyone with free time to analyze the problem ? :-)
> > 
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >> ---
> >>  src/libcamera/include/v4l2_device.h | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> >> index 474c05b..5fe3d82 100644
> >> --- a/src/libcamera/include/v4l2_device.h
> >> +++ b/src/libcamera/include/v4l2_device.h
> >> @@ -45,9 +45,9 @@ public:
> >>  	bool isOpen() const;
> >>  	void close();
> >>
> >> -	const char *driverName() const { return caps_.driver(); }
> >> -	const char *deviceName() const { return caps_.card(); }
> >> -	const char *busName() const { return caps_.bus_info(); }
> >> +	const std::string driverName() const { return std::string(caps_.driver()); }
> >> +	const std::string deviceName() const { return std::string(caps_.card()); }
> >> +	const std::string busName() const { return std::string(caps_.bus_info()); }
> > 
> > If we wanted to return an std::string, we should drop the const as the
> > string is returned by value.
> > 
> >>
> >>  private:
> >>  	std::string devnode_;