Re: [PATCH 5/8] v4l2-info: remove a strange sizeof usage

[Hans Verkuil <hverkuil@xs4all.nl>]

On 21/04/2021 09:20, Rosen Penev wrote:
> The array has a nullptr and 0 member for some reason. Remove and convert
> loop to a for range one.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  utils/common/v4l2-info.cpp | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/utils/common/v4l2-info.cpp b/utils/common/v4l2-info.cpp
> index cb3cb91f7..0359cf137 100644
> --- a/utils/common/v4l2-info.cpp
> +++ b/utils/common/v4l2-info.cpp
> @@ -3,6 +3,8 @@
>   * Copyright 2018 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
>   */
>  
> +#include <array>
> +
>  #include <v4l2-info.h>
>  
>  static std::string num2s(unsigned num, bool is_hex = true)
> @@ -411,21 +413,20 @@ std::string mbus2s(unsigned flags, bool is_hsv)
>  	return flags2s(flags, mbus_ycbcr_def);
>  }
>  
> -static const flag_def selection_targets_def[] = {
> -	{ V4L2_SEL_TGT_CROP_ACTIVE, "crop" },
> -	{ V4L2_SEL_TGT_CROP_DEFAULT, "crop_default" },
> -	{ V4L2_SEL_TGT_CROP_BOUNDS, "crop_bounds" },
> -	{ V4L2_SEL_TGT_COMPOSE_ACTIVE, "compose" },
> -	{ V4L2_SEL_TGT_COMPOSE_DEFAULT, "compose_default" },
> -	{ V4L2_SEL_TGT_COMPOSE_BOUNDS, "compose_bounds" },
> -	{ V4L2_SEL_TGT_COMPOSE_PADDED, "compose_padded" },
> -	{ V4L2_SEL_TGT_NATIVE_SIZE, "native_size" },
> -	{ 0, nullptr }

The idea of having this sentinel is that this makes it easy to add new
entries without having to update the array size.

> +static constexpr std::array<flag_def, 8> selection_targets_def{

Something you need to do here, adding a new flag means updating the size.

New flags are added regularly, so keeping that robust is a good idea IMHO.

If it were possible to write:

static constexpr std::array<flag_def> selection_targets_def{

i.e. without an explicit size, then this would make sense, but C++
doesn't allow this. And std::vector allocates the data on the heap,
which is less efficient as well.

Let's just keep using normal arrays in this case, they do the job
just fine. Just because you have a hammer, it doesn't mean everything
is now a nail :-)

Regards,

	Hans

> +	flag_def{ V4L2_SEL_TGT_CROP_ACTIVE, "crop" },
> +	flag_def{ V4L2_SEL_TGT_CROP_DEFAULT, "crop_default" },
> +	flag_def{ V4L2_SEL_TGT_CROP_BOUNDS, "crop_bounds" },
> +	flag_def{ V4L2_SEL_TGT_COMPOSE_ACTIVE, "compose" },
> +	flag_def{ V4L2_SEL_TGT_COMPOSE_DEFAULT, "compose_default" },
> +	flag_def{ V4L2_SEL_TGT_COMPOSE_BOUNDS, "compose_bounds" },
> +	flag_def{ V4L2_SEL_TGT_COMPOSE_PADDED, "compose_padded" },
> +	flag_def{ V4L2_SEL_TGT_NATIVE_SIZE, "native_size" },
>  };
>  
>  bool valid_seltarget_at_idx(unsigned i)
>  {
> -	return i < sizeof(selection_targets_def) / sizeof(selection_targets_def[0]) - 1;
> +	return i < selection_targets_def.size();
>  }
>  
>  unsigned seltarget_at_idx(unsigned i)
> @@ -437,13 +438,9 @@ unsigned seltarget_at_idx(unsigned i)
>  
>  std::string seltarget2s(__u32 target)
>  {
> -	int i = 0;
> -
> -	while (selection_targets_def[i].str != nullptr) {
> -		if (selection_targets_def[i].flag == target)
> -			return selection_targets_def[i].str;
> -		i++;
> -	}
> +	for (const auto &def : selection_targets_def)
> +		if (def.flag == target)
> +			return def.str;
>  	return std::string("Unknown (") + num2s(target) + ")";
>  }
>  
>