Concept ACK

Although I agree this is slightly better, I'm honestly not sure we should be doing refactors all over the place just for the sake of using C++17. It's always possible to introduce a subtle issue. We did have a "new code and code that is changed anyway" policy at the time of C++11 at least.

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Introduce deploymentstatus #19438 (Introduce deploymentstatus by ajtowns)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Concept ACK

While I agree that we shouldn't introduce C++17 just for the sake of it I think this one is a clear win since it removes a potentially unsafe idiom with a safer replacement. (To be clear: I think our current usages of this idiom are safe, but I think removing an unsafe idiom from the code base reduces the risk of accidental introduction of bugs due to said unsafe idiom in the future due to copy pasta, imitation, etc.)

sizeof(arr) / sizeof(arr[0]) is a potentially unsafe idiom due to this gotcha:

$ cat sizeof_gotcha.cpp
#include <iostream>

int f1(int arr[]) { return sizeof(arr) / sizeof(arr[0]); }

// Will give a compile time error:
// int f2(int arr[]) { return std::size(arr); }

int main() {
  int arr[]{1, 2, 3};
  std::cout << "sizeof(arr) / sizeof(arr[0]) = " << sizeof(arr) / sizeof(arr[0]) << "\n";
  std::cout << "sizeof(arr) / sizeof(arr[0]) in f1 = " << f1(arr) << "\n";
  std::cout << "std::size(arr) = " << std::size(arr) << "\n";
  // std::cout << "std::size(arr) in f2 = " << f2(arr) << "\n";
}

$ ./sizeof_gotcha
sizeof(arr) / sizeof(arr[0]) = 3
sizeof(arr) / sizeof(arr[0]) in f1 = 2
std::size(arr) = 3

Note the "unexpected" result from sizeof-based f1 due to pointer decay.

Note also how the std::size-based f2 errors out at compile time instead.

I've verified that this change is complete by comparing the suggested changes against:

$ git grep -E '(sizeof\(.*/.*sizeof\(|ARRAYLEN)' ":(exclude)src/leveldb/" ":(exclude)src/secp256k1/" \
      ":(exclude)src/univalue/" ":(exclude)src/crypto/"

sizeof(arr) / sizeof(arr[0]) is a potentially unsafe idiom due to this gotcha:

Yes, agree in this specific case.

Light code review ACK.

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

ACK after rebase

Rebased on master.

Force-pushed with suggestions of MarcoFalke, see #20429 (comment) and #20429 (comment)). Also updated the PR description.
Note due to reordering of the commits, its timestamps are also out-of-order now, let me know if this is a problem.

cr ACK e829c9a: patch looks correct