| Message ID | 20210421072035.4188497-1-rosenp@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/8] clang-tidy: use auto | expand |
On 21/04/2021 09:20, Rosen Penev wrote: > Allows usage of a single any_of instead of a raw loop. > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > --- > utils/v4l2-ctl/v4l2-ctl-common.cpp | 24 +++++------------------- > 1 file changed, 5 insertions(+), 19 deletions(-) > > diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp > index 17ad488dd..2b6dd6d13 100644 > --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp > +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp > @@ -116,28 +116,14 @@ void common_usage() > ); > } > > -static const char *prefixes[] = { > - "video", > - "radio", > - "vbi", > - "swradio", > - "v4l-subdev", > - "v4l-touch", > - "media", > - nullptr > +static constexpr std::array<const char *, 7> prefixes{ > + "video", "radio", "vbi", "swradio", "v4l-subdev", "v4l-touch", "media", > }; > > static bool is_v4l_dev(const char *name) > { > - for (unsigned i = 0; prefixes[i]; i++) { > - unsigned l = strlen(prefixes[i]); > - > - if (!memcmp(name, prefixes[i], l)) { > - if (isdigit(name[l])) > - return true; > - } > - } > - return false; > + return std::any_of(prefixes.begin(), prefixes.end(), > + [=](const char *prefix) { return !strcmp(name, prefix) && isdigit(name[strlen(prefix)]); }); Yuck. Besides, it is wrong AFAIKS since strcmp is not the same as memcmp. I like the original code, easier to understand than the replacement. So sue me :-) Regards, Hans > } > > static int calc_node_val(const char *s) > @@ -146,7 +132,7 @@ static int calc_node_val(const char *s) > > s = std::strrchr(s, '/') + 1; > > - for (unsigned i = 0; prefixes[i]; i++) { > + for (size_t i = 0; i < prefixes.size(); i++) { > unsigned l = strlen(prefixes[i]); > > if (!memcmp(s, prefixes[i], l)) { >
> On Apr 21, 2021, at 01:23, Hans Verkuil <hverkuil@xs4all.nl> wrote: > >> 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. Not following this. I assume it’s some C feature. > >> +static constexpr std::array<flag_def, 8> selection_targets_def{ > > Something you need to do here, adding a new flag means updating the size. I assume this is a small issue. It’s an immediate compile error anyway. > > 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. But this is possible. With C++17 :). That would necessitate a minimum of GCC6 though. > > 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) + ")"; >> } >> >> >
> On Apr 21, 2021, at 02:33, Hans Verkuil <hverkuil@xs4all.nl> wrote: > >> On 21/04/2021 11:19, Rosen Penev wrote: >> >>>> On Apr 21, 2021, at 01:23, Hans Verkuil <hverkuil@xs4all.nl> wrote: >>>> >>>> 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. >> Not following this. I assume it’s some C feature. > > Standard programming techique: > > https://en.wikipedia.org/wiki/Sentinel_value#:~:text=In%20computer%20programming%2C%20a%20sentinel,a%20loop%20or%20recursive%20algorithm. > >>> >>>> +static constexpr std::array<flag_def, 8> selection_targets_def{ >>> >>> Something you need to do here, adding a new flag means updating the size. >> I assume this is a small issue. It’s an immediate compile error anyway. > > Not if the size is larger that the number of initializers. That would leave > zeroed elements at the end. Ah yes that is a real problem. I’ve been bitten by it in the past. > >>> >>> 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. >> But this is possible. With C++17 :). That would necessitate a minimum of GCC6 though. > > How is that done in C++17? I didn't find anything about that. C++17 has class template argument deduction. It allows putting just std::array or std::vector or any other container really without the <>. Of course you have to allow it to deduce the arguments by putting them in the initializers. This is actually what I did in the previous cec-tuner patch. > > Regards, > > Hans > >>> >>> 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) + ")"; >>>> } >>>> >>>> >>> >
Le mercredi 21 avril 2021 à 11:33 +0200, Hans Verkuil a écrit : > On 21/04/2021 11:19, Rosen Penev wrote: > > > > > On Apr 21, 2021, at 01:23, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > > > > > > 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. > > Not following this. I assume it’s some C feature. > > Standard programming techique: > > https://en.wikipedia.org/wiki/Sentinel_value#:~:text=In%20computer%20programming%2C%20a%20sentinel,a%20loop%20or%20recursive%20algorithm. > > > > > > > > +static constexpr std::array<flag_def, 8> selection_targets_def{ > > > > > > Something you need to do here, adding a new flag means updating the size. > > I assume this is a small issue. It’s an immediate compile error anyway. > > Not if the size is larger that the number of initializers. That would leave > zeroed elements at the end. > > > > > > > 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. > > But this is possible. With C++17 :). That would necessitate a minimum of GCC6 though. > > How is that done in C++17? I didn't find anything about that. I believe this is similar to this code: https://git.linuxtv.org/libcamera.git/tree/src/gstreamer/gstlibcamera-utils.cpp#n15 But there I only used the fact that you can iterate it like any other collection. This is a recent feature I got told (I'm quite a C++ newby, and I like static arrays without passing and updating a size). > > Regards, > > Hans > > > > > > > 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) + ")"; > > > > } > > > > > > > > > > > >
diff --git a/utils/cec-ctl/cec-ctl.cpp b/utils/cec-ctl/cec-ctl.cpp index 107dcfab2..c17265a5c 100644 --- a/utils/cec-ctl/cec-ctl.cpp +++ b/utils/cec-ctl/cec-ctl.cpp @@ -1833,7 +1833,7 @@ static __u16 parse_phys_addr_from_edid(const char *edid_path) static void *thread_edid_poll(void *arg) { - struct node *node = static_cast<struct node *>(arg); + auto node = static_cast<struct node *>(arg); __u16 phys_addr; bool has_edid; char dummy;
Found with modernize-use-auto Signed-off-by: Rosen Penev <rosenp@gmail.com> --- utils/cec-ctl/cec-ctl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)