Universal revisions based on clang-tidy

[Lestropie]

Since #2745, clang-tidy was introduced.
It is run only on proposed changes within PRs, not across the whole code base, since the latter would be overly burdensome.
We can however elect to take any recommendations that it makes based on isolated code, and apply the same changes across the code base, eg. in cases where instances of such can be found through a regex search or where it's known that a particular design pattern is used in a finite number of places.
This issue will serve to facilitate listing clang-tidy proposals that may be worthwhile revising the code base.
Hopefully it will doubly serve to show clang-tidy proposals that we are aware of but may not necessarily resolve in any unrelated PR, so that those less experienced will not panic if they see them.

---

• cppcoreguidelines-pro-bounds-array-to-pointer-decay
  Exemplified in Disable clang format cli take2 #2818.
  Really does not like the way that strings and lists-of-strings are handled in the CLI; particularly for Argument::type_choice(), but also for eg. App::DESCRIPTION.
  Would be best resolved comprehensively with Back-end: Remove all char* / char** usages #2111.

• cppcoreguidelines-macro-usage
  Exemplified in Disable clang format cli take2 #2818 for cmd/mrview.cpp, but macro functions are likely used in a number of other places.

• readability-container-size-empty
  "warning: the 'empty' method should be used to check for emptiness instead of 'size'"
  First seen in dwicat: Support different voxel grids #2702.
  Might be most applicable to cases where get_option_value() should instead be used. The case in dwicat: Support different voxel grids #2702 is an enum class, which from memory prevented get_option_value() from being used. But probably also many other instances, and potentially something that could be found via grep.
  Addressed in Check emptiness of containers using empty() #2829.

• readability-uppercase-literal-suffix
  Shown in Check emptiness of containers using empty() #2829, though I've seen it elsewhere also.
  Had never encountered the recommendation that datatype suffices should be capitalised. This is another that might be possible to automatically correct across the code base, at least as a first pass; it's possible that some casts to single precision predate the transition of default_type to double precision, and therefore in fact the suffix should be removed.

• misc-const-correctness
  Shown in Check emptiness of containers using empty() #2829, but also one I've seen elsewhere.
  This might be possible to accept automatic changes with some manual revisions to tweak formatting. As long as both major compilers are happy after the change.
  Only catch if trying to apply an automatic fix here is that it seems to like placing the const qualifier after the type, rather than before. I presume this is because it better disambiguates cases like const pointers to const objects, but it still looks alien to me currently.

• readability-implicit-bool-conversion
  This includes comparing pointers to nullptr and checking if integers are greater than zero. The occurrences I've seen I've agreed with, so might be applicable across the board.

• cppcoreguidelines-init-variables
  There are certainly instances where the content of a variable gets immediately set through some function call, and so the lack of initialization only persists for one line of code. It would nevertheless be preferable to initialise all variables anyway to shut this one up. Indeed in some circumstances I've explicitly used std::numeric_limits::signaling_NaN() in initialisers; we may not be catching such, but it clearly signals within the code that it's not intended to be a valid value (unlike other pieces of code where we intentionally exploit NaNs).

• readability-make-member-function-const

• concurrency-mt-unsafe
  Somewhat related to Document what resources should be protected by mutexes #2795.

• readability-const-return-type
  Converse to prior listing, using const where it shouldn't be used. Also a potential flag that the return type may have been intended to be a reference but the ampersand was erroneously omitted.

[daljit46]

readability-container-size-empty can be applied using clang-tidy -fix, but the other two would too risky to change using automation. In particular the first would most likely need manual supervision on all instances of const char* as we would need to think about the lifetimes of strings.
I'm in favour of this though, so I think a gradual adoption of std::string and perhaps std::string_view would be ideal. Perhaps, we could start doing this with all application level code (i.e. all code in cmd).

[Lestropie]

I wouldn't want to accept the automated changes for readability-container-size-empty either; it suggests changing:
value = opt.size() ? A : B;
to:
value = !opt.empty() ? A : B;
, whereas preferable would be:
value = opt.empty() ? B : A;

Edit: Wouldn't want to blindly accept the automated change; happy to manually revise #2829 if necessary.

[Lestropie]

On #2702 I'm currently getting a lot of bugprone-narrowing-conversions going from unsigned to signed array indices. clang-tidy seems to be even more aggressive in this respect than the current state of the compilers (which themselves have progressively flagged more code in this respect). Not sure if this is one we might actually want to disable in clang-tidy, as it could produce a lot of noise resulting in people ignoring the meaningful suggestions.

[daljit46]

I agree on that example.

Edit: Wouldn't want to blindly accept the automated change; happy to manually revise #2829 if necessary.

That'd be great!

On #2702 I'm currently getting a lot of bugprone-narrowing-conversions going from unsigned to signed array indices.

Although I understand that this check is noisy, my opinion is that we should make an effort to follow its advice. If a conversion is benign (which often can be a deceiving matter), an explicit cast (preferably static_cast or dynamic_cast) would be best. This tells the reader that the author is intentionally allowing the conversion.

[Lestropie]

Something to think about as #2829 hopefully gets merged:

It might be the case that, for those clang-tidy checks that we do decide to adopt across the repository, we choose to add a CI check that runs clang-tidy -fix just for those specific features, which result in CI failure rather than a bot comment. It would appear alongside any clang-format errors, so it's only kind of a half developer step addition.

Where I'm not sure if this would work is in precommit hooks. It often takes a long time for clang-tidy Action results to come in, so there's a chance it might be more computationally intensive to run at commit time than clang-format?

[Lestropie]

Added a few to the list in reviewing #2877.