refactor: replace sizeof(a)/sizeof(a[0]) by ARRAYLEN(a) #19626

1. 
   theStack commented at 2:24 pm on July 30, 2020: member

   This tiny refactoring PR replaces all occurences of sizeof(x)/sizeof(x[0]) (or sizeof(x)/sizeof(*x), respectively) with the macro ARRAYLEN(x). While the pattern to determine the array of a length is a very common one and should be familiar for every C or C++ programmer, I think readability is still improved if it is hidden behind a macro. And since we have already ARRAYLEN in the codebase since the beginning (indeed Satoshi already had it in v0.1.0, see https://github.com/trottier/original-bitcoin/blob/master/src/util.h#L27), why not use it consistently?

   The instances to replace were discovered via

   $ grep -Ir "sizeof.*/.*sizeof" src/*

   If this gets a Concept ACK, one could probably discuss if the header strencodings.h is really the right location for this macro – it doesn’t have anything to do with strings at all. Maybe util/macro.h would be a more appropriate place.

2. refactor: replace sizeof(a)/sizeof(a[0]) by ARRAYLEN(a) 3b21535e0f
3. 
   laanwj commented at 2:32 pm on July 30, 2020: member

   It does make me wonder why the macro is in strencodings.h, which now needs to be included everywwhere. Maybe it would be better to move it to a separate header first.
4. laanwj added the label Refactoring on Jul 30, 2020
5. 
   theStack commented at 2:40 pm on July 30, 2020: member

   It was moved from src/util.h to src/utilstrencodings.h (which was later moved to src/util/strencodings.h) back in 2014: ad49c256c33bfe4088fd3c7ecb7d28cb81a8fc70
6. 
   kristapsk commented at 3:09 pm on July 30, 2020: contributor

   There is a better way to do it in C++ actually, see https://github.com/kristapsk/resclib/blob/0df895a9f02d393791590aa79c0022476ac1721a/include/stdlib.h#L51.
7. 
   kristapsk commented at 3:22 pm on July 30, 2020: contributor

   Also, _countof is supported by default in Microsoft C/C++ lib.
8. 
   sipa commented at 4:00 pm on July 30, 2020: member

   C++17 has std::size for this. We could backport it, or just wait.
9. 
   DrahtBot commented at 8:28 pm on July 30, 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:

   • #19528 (rpc: Assert that RPCArg names are equal to CRPCCommand ones (misc) by MarcoFalke)
   • #16439 (cli/gui: support “@height” in place of blockhash for getblock on client side by ajtowns)
   • #16333 (test: Set BIP34Height = 2 for regtest by MarcoFalke)

   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.

10. in src/rpc/server.cpp:260 in 3b21535e0f

    256@@ -257,7 +257,7 @@ static const CRPCCommand vRPCCommands[] =
    257 CRPCTable::CRPCTable()
    258 {
    259     unsigned int vcidx;
    260-    for (vcidx = 0; vcidx < (sizeof(vRPCCommands) / sizeof(vRPCCommands[0])); vcidx++)
    261+    for (vcidx = 0; vcidx < ARRAYLEN(vRPCCommands); vcidx++)
    

    ---

    ---

    

    MarcoFalke commented at 4:55 am on July 31, 2020:

    The idx isn’t needed, so a C++11 range-based for loop would be better suited.

    See conflicting pull #19528

11. 
    laanwj commented at 10:30 am on August 2, 2020: member

    C++17 has std::size for this. We could backport it, or just wait.

    Let’s just wait IMO. There is no hurry in the world to do this. The current solution works perfectly for the meantime.

12. MarcoFalke added this to the milestone 0.22.0 on Aug 2, 2020
13. 
    theStack commented at 1:37 pm on August 2, 2020: member

    Thanks for all of your feedback. The initial idea of this PR was just to make use of a macro that is already there to increase readability of the code. The answers in the code suggested some more ideas, each of them valid – to summarize:

    • moving the macro from strencodings.h to a more appropriate header (probably a new one)
    • changing from the C way of determining the size of an array to the typesafe C++ way using templates (available via std::size with C++17)
    • replacing indexed accesses to arrays by C++11 range-based for loops, eliminating the need of determining the number of elements

    I agree that it makes sense to just wait for C++17 and not backport std::size. Also the other two points can be done then, or for the case of the range-based for loops, whenever that piece of code is touched for another reason, like in #19528. Closing this PR now.

14. theStack closed this on Aug 2, 2020

    ---

15. MarcoFalke referenced this in commit cd66d8b1d8 on Feb 18, 2021
16. DrahtBot locked this on Feb 15, 2022

Contributors
theStack laanwj kristapsk sipa DrahtBot MarcoFalke

Labels
Refactoring

Milestone
22.0