refactor: replace (sizeof(a)/sizeof(a[0])) with C++17 std::size #20429

1. 
   theStack commented at 0:32 am on November 20, 2020: member

   This refactoring PR picks up the idea of #19626 and replaces all occurences of sizeof(x)/sizeof(x[0]) (or sizeof(x)/sizeof(*x), respectively) with the now-available C++17 std::size (as suggested by sipa), making the macro ARRAYLEN obsolete.

   As preparation for this, two other changes are done to eliminate sizeof(x)/sizeof(x[0]) usage:

   • all places where arrays are iterated via an index are changed to use C++11 range-based for loops If the index’ only purpose is to access the array element (as suggested by MarcoFalke).
   • std::vector initializations are done via std::begin and std::end rather than using pointer arithmetic to calculate the end (also suggested by MarcoFalke).
2. theStack renamed this:
   refactor: replace (sizeof(a)/sizeof(a[0]) with C++17 std::size
   refactor: replace (sizeof(a)/sizeof(a[0])) with C++17 std::size
   on Nov 20, 2020
3. fanquake added the label Refactoring on Nov 20, 2020
4. 
   fanquake commented at 0:53 am on November 20, 2020: member

   Concept ACK
5. 
   laanwj commented at 2:34 am on November 20, 2020: member

   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.
6. 
   DrahtBot commented at 5:09 am on November 20, 2020: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #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.

7. 
   practicalswift commented at 8:57 am on November 20, 2020: contributor

   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:

    0$ cat sizeof_gotcha.cpp
    1#include <iostream>
    2
    3int f1(int arr[]) { return sizeof(arr) / sizeof(arr[0]); }
    4
    5// Will give a compile time error:
    6// int f2(int arr[]) { return std::size(arr); }
    7
    8int main() {
    9  int arr[]{1, 2, 3};
   10  std::cout << "sizeof(arr) / sizeof(arr[0]) = " << sizeof(arr) / sizeof(arr[0]) << "\n";
   11  std::cout << "sizeof(arr) / sizeof(arr[0]) in f1 = " << f1(arr) << "\n";
   12  std::cout << "std::size(arr) = " << std::size(arr) << "\n";
   13  // std::cout << "std::size(arr) in f2 = " << f2(arr) << "\n";
   14}
   

   0$ ./sizeof_gotcha
   1sizeof(arr) / sizeof(arr[0]) = 3
   2sizeof(arr) / sizeof(arr[0]) in f1 = 2
   3std::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:

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

8. brunoerg approved
9. 
   laanwj commented at 11:03 am on November 22, 2020: member

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

   Yes, agree in this specific case.

   Light code review ACK.

10. 
    DrahtBot commented at 6:58 pm on January 28, 2021: member

    🐙 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”.

11. DrahtBot added the label Needs rebase on Jan 28, 2021
12. fanquake added the label Waiting for author on Jan 31, 2021
13. 
    fanquake commented at 10:14 am on January 31, 2021: member

    ACK after rebase
14. theStack force-pushed on Jan 31, 2021
15. 
    theStack commented at 11:15 am on January 31, 2021: member

    Rebased on master.
16. fanquake removed the label Needs rebase on Jan 31, 2021
17. fanquake removed the label Waiting for author on Jan 31, 2021
18. in src/bench/data.cpp:11 in 7841a099b1 outdated

     7@@ -8,7 +8,7 @@ namespace benchmark {
     8 namespace data {
     9 
    10 #include <bench/data/block413567.raw.h>
    11-const std::vector<uint8_t> block413567{block413567_raw, block413567_raw + sizeof(block413567_raw) / sizeof(block413567_raw[0])};
    12+const std::vector<uint8_t> block413567{block413567_raw, block413567_raw + std::size(block413567_raw)};
    

    ---

    ---

    

    MarcoFalke commented at 12:28 pm on January 31, 2021:

    0const std::vector<uint8_t> block413567{std::begin(block413567_raw), std::end(block413567_raw)};
    

    seem overly verbose to do pointer arithmetic here. Same for all other occurrences in this pull

    ---

    

    theStack commented at 4:44 pm on January 31, 2021:

    Done, added an extra commit for this, again before the std::size substitution to minimize retouching of lines.
19. refactor: iterate arrays via C++11 range-based for loops if idx is not needed 63d4ee1968
20. refactor: init vectors via std::{begin,end} to avoid pointer arithmetic 365539c846
21. refactor: replace sizeof(a)/sizeof(a[0]) by std::size (C++17)

    Removes the macro ARRAYLEN and also substitutes all other uses of the same
    "sizeof(a)/sizeof(a[0])" pattern by std::size, available since C++17.

    e829c9afbf
22. in src/qt/networkstyle.cpp:83 in 7841a099b1 outdated

    79@@ -80,14 +80,12 @@ NetworkStyle::NetworkStyle(const QString &_appName, const int iconColorHueShift,
    80 const NetworkStyle* NetworkStyle::instantiate(const std::string& networkId)
    81 {
    82     std::string titleAddText = networkId == CBaseChainParams::MAIN ? "" : strprintf("[%s]", networkId);
    83-    for (unsigned x=0; x<std::size(network_styles); ++x)
    

    ---

    ---

    

    MarcoFalke commented at 12:30 pm on January 31, 2021:

    Would be nice to re-order the commits, so that the same line of code isn’t changed twice in the same pull

    ---

    

    theStack commented at 4:44 pm on January 31, 2021:

    Good idea, done.
23. theStack force-pushed on Jan 31, 2021
24. 
    theStack commented at 4:47 pm on January 31, 2021: member

    Force-pushed with suggestions of MarcoFalke, see #20429 (review) and #20429 (review)). 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.
25. 
    practicalswift commented at 6:35 pm on January 31, 2021: contributor

    cr ACK e829c9afbf75e930db6c3fe77a269b0af5e7a3ad: patch looks correct
26. fanquake approved
27. 
    fanquake commented at 12:13 pm on February 17, 2021: member

    ACK e829c9afbf75e930db6c3fe77a269b0af5e7a3ad
28. MarcoFalke approved
29. 
    MarcoFalke commented at 6:53 am on February 18, 2021: member

    review ACK e829c9afbf75e930db6c3fe77a269b0af5e7a3ad 🌩

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3review ACK e829c9afbf75e930db6c3fe77a269b0af5e7a3ad 🌩
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUjxnAv+IIfvDbq/F7CYc56GGB/cpjXOouSirKbLpzFCtLN5qf3Nm8YaC2D4falz
     8FjP6yiVo2wclJYOwKA2hJAwAADynBBOxWKyol2iT7ps6QFthPc+AyXMph3JO49t4
     9XBh0Jx6oRz/LyT4A/gWsHsMoufDhIh+P9xJ0gyG+0EIuiOIYed3y5JOQdaCecRUg
    10Fz0wdTjxIoyGI4NiHOc3a8wQWyuLWjzb6X9LGGXjw2vA+muP8343CPjzbUqcrbSf
    11UJWszfJZCdzTvgJzM3PHWEv8xqeJXuMwLNiUTiBzliBaDbkFw8BkV1hO5Xufbxye
    12JdQjS08db1gfOBnQT4jm+Np5c3K6OGsmlH7L1nOChaDBJh2Zi8HirUMOrgDZ6vii
    132K/ThPNnvoLLhcNqRk51/F2jvjkkWwWNzfXux6H2t7K4EBckznGmTAD+BRO8ZvMi
    14IbvOwlzKr+ytFV6+Dzt9bZdnOtl+P56pswjd2G1LfWOa+qExOiCKFZZA1hbiCdH4
    15LbTt/Lm2
    16=sj81
    17-----END PGP SIGNATURE-----
    

    Timestamp of file with hash 299d77a070c17af50cb1cc6964e89dfae768fc6598b33fd733eacddbbf5c15be -

30. MarcoFalke merged this on Feb 18, 2021
31. MarcoFalke closed this on Feb 18, 2021

    ---

32. sidhujag referenced this in commit 807047f12d on Feb 18, 2021
33. deadalnix referenced this in commit b3c20cc418 on Mar 10, 2022
34. DrahtBot locked this on Aug 16, 2022

Contributors
theStack fanquake laanwj DrahtBot practicalswift brunoerg MarcoFalke

Labels
Refactoring