[libcamera-devel,v2,3/4] libcamera: utils: Use internal basename implementation.

829 diff mbox series

Differing implementations of basename() exist, some of which may modify
the content of the string passed as an argument.

The implementation of basename() is trivial, thus to support different
toolchains, provide our own version which accepts and returns a const
char*.

Update the call sites to use the new implementation.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/include/utils.h |  2 ++
 src/libcamera/log.cpp         |  2 +-
 src/libcamera/meson.build     |  1 +
 src/libcamera/utils.cpp       | 46 +++++++++++++++++++++++++++++++++++
 4 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100644 src/libcamera/utils.cpp

Hi Kieran,

Thank you for the patch.

On Mon, Apr 01, 2019 at 06:03:14PM +0700, Kieran Bingham wrote:
> Differing implementations of basename() exist, some of which may modify
> the content of the string passed as an argument.
> 
> The implementation of basename() is trivial, thus to support different
> toolchains, provide our own version which accepts and returns a const
> char*.

s/char\*/char */

> 
> Update the call sites to use the new implementation.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/include/utils.h |  2 ++
>  src/libcamera/log.cpp         |  2 +-
>  src/libcamera/meson.build     |  1 +
>  src/libcamera/utils.cpp       | 46 +++++++++++++++++++++++++++++++++++
>  4 files changed, 50 insertions(+), 1 deletion(-)
>  create mode 100644 src/libcamera/utils.cpp
> 
> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> index 73fa2e69b029..1b2a62c0fda7 100644
> --- a/src/libcamera/include/utils.h
> +++ b/src/libcamera/include/utils.h
> @@ -15,6 +15,8 @@ namespace libcamera {
>  
>  namespace utils {
>  
> +const char *basename(const char *path);
> +
>  /* C++11 doesn't provide std::make_unique */
>  template<typename T, typename... Args>
>  std::unique_ptr<T> make_unique(Args&&... args)
> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> index 26ebf410a7a9..eb444c31857d 100644
> --- a/src/libcamera/log.cpp
> +++ b/src/libcamera/log.cpp
> @@ -438,7 +438,7 @@ void LogMessage::init(const char *fileName, unsigned int line)
>  
>  	msgStream_ << " " << log_severity_name(severity_);
>  	msgStream_ << " " << category_.name();
> -	msgStream_ << " " << basename(fileName) << ":" << line << " ";
> +	msgStream_ << " " << utils::basename(fileName) << ":" << line << " ";
>  }
>  
>  LogMessage::~LogMessage()
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 8384cd0af451..863cb60d4b90 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -16,6 +16,7 @@ libcamera_sources = files([
>      'signal.cpp',
>      'stream.cpp',
>      'timer.cpp',
> +    'utils.cpp',
>      'v4l2_device.cpp',
>      'v4l2_subdevice.cpp',
>  ])
> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> new file mode 100644
> index 000000000000..70936e36c5d5
> --- /dev/null
> +++ b/src/libcamera/utils.cpp
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * utils.cpp - Miscellaneous utility functions
> + */
> +
> +#include <string.h>
> +
> +#include "utils.h"
> +
> +/**
> + * \file utils.h
> + * \brief Miscellaneous utility functions
> + */
> +
> +namespace libcamera {
> +
> +namespace utils {
> +
> +/**
> + * \def ARRAY_SIZE(array)
> + * \brief Determine the number of elemnents in the static array.

s/elemnents/elements/

> + */
> +
> +/**
> + * \brief Strip the directory prefix from the path

Missing "\param[in] path" and \return.

> + *
> + * basename is implemented differently across different toolchains.
> + * Ensure consistency by providing our own implementation.

I would instead tell what behaviour we provide. For instance you could
mention that this implementation matches the one provided by the GNU
libc.

> + */
> +const char *basename(const char *path)
> +{
> +       const char *base = strrchr(path, '/');
> +       return base ? base + 1 : path;
> +}
> +
> +
> +/**
> + * \fn libcamera::utils::make_unique(Args &&... args)
> + * \brief Construct an object and return a std::unique ptr to it

"Constructs an object of type T and wraps it in a std::unique_ptr." to
match the documentation from
https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique ?

> + */
> +
> +} /* namespace utils */
> +
> +} /* namespace libcamera */

Hi Laurent,

On 01/04/2019 12:10, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Mon, Apr 01, 2019 at 06:03:14PM +0700, Kieran Bingham wrote:
>> Differing implementations of basename() exist, some of which may modify
>> the content of the string passed as an argument.
>>
>> The implementation of basename() is trivial, thus to support different
>> toolchains, provide our own version which accepts and returns a const
>> char*.
> 
> s/char\*/char */
> 
>>
>> Update the call sites to use the new implementation.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  src/libcamera/include/utils.h |  2 ++
>>  src/libcamera/log.cpp         |  2 +-
>>  src/libcamera/meson.build     |  1 +
>>  src/libcamera/utils.cpp       | 46 +++++++++++++++++++++++++++++++++++
>>  4 files changed, 50 insertions(+), 1 deletion(-)
>>  create mode 100644 src/libcamera/utils.cpp
>>
>> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
>> index 73fa2e69b029..1b2a62c0fda7 100644
>> --- a/src/libcamera/include/utils.h
>> +++ b/src/libcamera/include/utils.h
>> @@ -15,6 +15,8 @@ namespace libcamera {
>>  
>>  namespace utils {
>>  
>> +const char *basename(const char *path);
>> +
>>  /* C++11 doesn't provide std::make_unique */
>>  template<typename T, typename... Args>
>>  std::unique_ptr<T> make_unique(Args&&... args)
>> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
>> index 26ebf410a7a9..eb444c31857d 100644
>> --- a/src/libcamera/log.cpp
>> +++ b/src/libcamera/log.cpp
>> @@ -438,7 +438,7 @@ void LogMessage::init(const char *fileName, unsigned int line)
>>  
>>  	msgStream_ << " " << log_severity_name(severity_);
>>  	msgStream_ << " " << category_.name();
>> -	msgStream_ << " " << basename(fileName) << ":" << line << " ";
>> +	msgStream_ << " " << utils::basename(fileName) << ":" << line << " ";
>>  }
>>  
>>  LogMessage::~LogMessage()
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index 8384cd0af451..863cb60d4b90 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -16,6 +16,7 @@ libcamera_sources = files([
>>      'signal.cpp',
>>      'stream.cpp',
>>      'timer.cpp',
>> +    'utils.cpp',
>>      'v4l2_device.cpp',
>>      'v4l2_subdevice.cpp',
>>  ])
>> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
>> new file mode 100644
>> index 000000000000..70936e36c5d5
>> --- /dev/null
>> +++ b/src/libcamera/utils.cpp
>> @@ -0,0 +1,46 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2019, Google Inc.
>> + *
>> + * utils.cpp - Miscellaneous utility functions
>> + */
>> +
>> +#include <string.h>
>> +
>> +#include "utils.h"
>> +
>> +/**
>> + * \file utils.h
>> + * \brief Miscellaneous utility functions
>> + */
>> +
>> +namespace libcamera {
>> +
>> +namespace utils {
>> +
>> +/**
>> + * \def ARRAY_SIZE(array)
>> + * \brief Determine the number of elemnents in the static array.
> 
> s/elemnents/elements/

Doh. Fixed.


> 
>> + */
>> +
>> +/**
>> + * \brief Strip the directory prefix from the path
> 
> Missing "\param[in] path" and \return.
> 
>> + *
>> + * basename is implemented differently across different toolchains.
>> + * Ensure consistency by providing our own implementation.
> 
> I would instead tell what behaviour we provide. For instance you could
> mention that this implementation matches the one provided by the GNU
> libc.
> 

Sure sounds good.


>> + */
>> +const char *basename(const char *path)
>> +{
>> +       const char *base = strrchr(path, '/');
>> +       return base ? base + 1 : path;
>> +}
>> +
>> +
>> +/**
>> + * \fn libcamera::utils::make_unique(Args &&... args)
>> + * \brief Construct an object and return a std::unique ptr to it
> 
> "Constructs an object of type T and wraps it in a std::unique_ptr." to
> match the documentation from
> https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique ?
> 

Updated.


>> + */
>> +
>> +} /* namespace utils */
>> +
>> +} /* namespace libcamera */
>