refactor: replace sizeof(a)/sizeof(a[0]) by ARRAYLEN(a) #19626
Conversation
|
It does make me wonder why the macro is in |
|
It was moved from |
|
There is a better way to do it in C++ actually, see https://github.com/kristapsk/resclib/blob/0df895a9f02d393791590aa79c0022476ac1721a/include/stdlib.h#L51. |
|
Also, |
|
C++17 has |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
| @@ -257,7 +257,7 @@ static const CRPCCommand vRPCCommands[] = | |||
| CRPCTable::CRPCTable() | |||
| { | |||
| unsigned int vcidx; | |||
| for (vcidx = 0; vcidx < (sizeof(vRPCCommands) / sizeof(vRPCCommands[0])); vcidx++) | |||
| for (vcidx = 0; vcidx < ARRAYLEN(vRPCCommands); vcidx++) | |||
There was a problem hiding this comment.
The idx isn't needed, so a C++11 range-based for loop would be better suited.
See conflicting pull #19528
Let's just wait IMO. There is no hurry in the world to do this. The current solution works perfectly for the meantime. |
|
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:
I agree that it makes sense to just wait for C++17 and not backport |
…td::size e829c9a refactor: replace sizeof(a)/sizeof(a[0]) by std::size (C++17) (Sebastian Falbesoner) 365539c refactor: init vectors via std::{begin,end} to avoid pointer arithmetic (Sebastian Falbesoner) 63d4ee1 refactor: iterate arrays via C++11 range-based for loops if idx is not needed (Sebastian Falbesoner) Pull request description: 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`](https://en.cppreference.com/w/cpp/iterator/size) (as [suggested by sipa](bitcoin/bitcoin#19626 (comment))), 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](bitcoin/bitcoin#19626 (comment))). * `std::vector` initializations are done via `std::begin` and `std::end` rather than using pointer arithmetic to calculate the end (also [suggested by MarcoFalke](bitcoin/bitcoin#20429 (comment))). ACKs for top commit: practicalswift: cr ACK e829c9a: patch looks correct fanquake: ACK e829c9a MarcoFalke: review ACK e829c9a 🌩 Tree-SHA512: b01d32c04b9e04d562b7717cae00a651ec9a718645047a90761be6959e0cc2adbd67494e058fe894641076711bb09c3b47a047d0275c736f0b2218e1ce0d193d
This tiny refactoring PR replaces all occurences of
sizeof(x)/sizeof(x[0])(orsizeof(x)/sizeof(*x), respectively) with the macroARRAYLEN(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 alreadyARRAYLENin 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.his really the right location for this macro -- it doesn't have anything to do with strings at all. Maybeutil/macro.hwould be a more appropriate place.