New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[tests] util_tests.cpp: actually check ignored args #11997
Conversation
cbf9165
to
4e196a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 4e196a9.
src/test/util_tests.cpp
Outdated
@@ -121,7 +121,8 @@ BOOST_AUTO_TEST_CASE(util_ParseParameters) | |||
testArgs.ParseParameters(1, (char**)argv_test); | |||
BOOST_CHECK(testArgs.GetMapArgs().empty() && testArgs.GetMapMultiArgs().empty()); | |||
|
|||
testArgs.ParseParameters(5, (char**)argv_test); | |||
static_assert(7 * sizeof(argv_test[0]) == sizeof(argv_test), "argv_test length has changed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO remove this assert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assert doesn't have any runtime cost, and documents/enforces the change as well as helping avoid the same mistake happening again, so seems better to have it to me?
An alternative approach is using templates to directly determine the size of the array:
template<size_t N>
inline void ParseParameters(int argc, const char* (&argv)[N])
{
assert(argc >= 0 && (size_t) argc < N);
ArgsManager::ParseParameters(argc, (char**) argv);
}
template<size_t N>
inline void ParseParameters(const char* (&argv)[N])
{
ArgsManager::ParseParameters(N, (char**) argv);
}
...
testArgs.ParseParameters(1, argv_test); // to test only some args
testArgs.ParseParameters(argv_test); // to test all args
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since passing an argc
that does not specify the size of argv
is only ever used in tests, you could pass argv_test.size()
instead of 7
by switching to const std::array<const char*, 7> argv_test = {... braced-init-list ...}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes it robust to future changes, but not robust to future changes that remove the trailing or the two trailing args. Imo such buggy changes must be covered by code review. Not excessive documentation. Note that we are talking about a function that spans about 20 lines... If code review does not work on that scale we have far worse issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
Concept ACK, but agree with @promag |
An array with 7 elements was setup for checking argument parsing, but was passed to ParseParamaeters with argc=5, meaning the interpretation of the last two arguments was never actually checked.
4e196a9
to
c99a3c3
Compare
|
c99a3c3 [tests] util_tests.cpp: actually check ignored args (Anthony Towns) Pull request description: An array with 7 elements was setup for checking argument parsing, but was passed to ParseParamaeters with argc=5, meaning the interpretation of the last two arguments was never actually checked. Tree-SHA512: 7b81fde49742e524f1bb67e2ec084f5909ae36125f237f0210df4587c62e5a5a8f277f13543f0a85ad145c4bb80d62339a7d50d7ed41659df318c8198ea7f428
Summary: c99a3c3 [tests] util_tests.cpp: actually check ignored args (Anthony Towns) Pull request description: An array with 7 elements was setup for checking argument parsing, but was passed to ParseParamaeters with argc=5, meaning the interpretation of the last two arguments was never actually checked. Tree-SHA512: 7b81fde49742e524f1bb67e2ec084f5909ae36125f237f0210df4587c62e5a5a8f277f13543f0a85ad145c4bb80d62339a7d50d7ed41659df318c8198ea7f428 Backport of Core PR11997: bitcoin/bitcoin#11997 Test Plan: make check Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3707
c99a3c3 [tests] util_tests.cpp: actually check ignored args (Anthony Towns) Pull request description: An array with 7 elements was setup for checking argument parsing, but was passed to ParseParamaeters with argc=5, meaning the interpretation of the last two arguments was never actually checked. Tree-SHA512: 7b81fde49742e524f1bb67e2ec084f5909ae36125f237f0210df4587c62e5a5a8f277f13543f0a85ad145c4bb80d62339a7d50d7ed41659df318c8198ea7f428
c99a3c3 [tests] util_tests.cpp: actually check ignored args (Anthony Towns) Pull request description: An array with 7 elements was setup for checking argument parsing, but was passed to ParseParamaeters with argc=5, meaning the interpretation of the last two arguments was never actually checked. Tree-SHA512: 7b81fde49742e524f1bb67e2ec084f5909ae36125f237f0210df4587c62e5a5a8f277f13543f0a85ad145c4bb80d62339a7d50d7ed41659df318c8198ea7f428
c99a3c3 [tests] util_tests.cpp: actually check ignored args (Anthony Towns) Pull request description: An array with 7 elements was setup for checking argument parsing, but was passed to ParseParamaeters with argc=5, meaning the interpretation of the last two arguments was never actually checked. Tree-SHA512: 7b81fde49742e524f1bb67e2ec084f5909ae36125f237f0210df4587c62e5a5a8f277f13543f0a85ad145c4bb80d62339a7d50d7ed41659df318c8198ea7f428
c99a3c3 [tests] util_tests.cpp: actually check ignored args (Anthony Towns) Pull request description: An array with 7 elements was setup for checking argument parsing, but was passed to ParseParamaeters with argc=5, meaning the interpretation of the last two arguments was never actually checked. Tree-SHA512: 7b81fde49742e524f1bb67e2ec084f5909ae36125f237f0210df4587c62e5a5a8f277f13543f0a85ad145c4bb80d62339a7d50d7ed41659df318c8198ea7f428
c99a3c3 [tests] util_tests.cpp: actually check ignored args (Anthony Towns) Pull request description: An array with 7 elements was setup for checking argument parsing, but was passed to ParseParamaeters with argc=5, meaning the interpretation of the last two arguments was never actually checked. Tree-SHA512: 7b81fde49742e524f1bb67e2ec084f5909ae36125f237f0210df4587c62e5a5a8f277f13543f0a85ad145c4bb80d62339a7d50d7ed41659df318c8198ea7f428
c99a3c3 [tests] util_tests.cpp: actually check ignored args (Anthony Towns) Pull request description: An array with 7 elements was setup for checking argument parsing, but was passed to ParseParamaeters with argc=5, meaning the interpretation of the last two arguments was never actually checked. Tree-SHA512: 7b81fde49742e524f1bb67e2ec084f5909ae36125f237f0210df4587c62e5a5a8f277f13543f0a85ad145c4bb80d62339a7d50d7ed41659df318c8198ea7f428
c99a3c3 [tests] util_tests.cpp: actually check ignored args (Anthony Towns) Pull request description: An array with 7 elements was setup for checking argument parsing, but was passed to ParseParamaeters with argc=5, meaning the interpretation of the last two arguments was never actually checked. Tree-SHA512: 7b81fde49742e524f1bb67e2ec084f5909ae36125f237f0210df4587c62e5a5a8f277f13543f0a85ad145c4bb80d62339a7d50d7ed41659df318c8198ea7f428
An array with 7 elements was setup for checking argument parsing, but
was passed to ParseParamaeters with argc=5, meaning the interpretation
of the last two arguments was never actually checked.