refactor: Remove unused and fragile string interface from arith_uint256 #28924
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
DrahtBot
changed the title
| @@ -95,25 +96,25 @@ BOOST_AUTO_TEST_CASE( basics ) // constructors, equality, inequality | |||
| BOOST_CHECK(ZeroL == (OneL << 256)); | |||
|
|
|||
| // String Constructor and Copy Constructor | |||
There was a problem hiding this comment.
This comment should be adjusted, since there is no arith_uint256(const std::string& str) constructor anymore.
There was a problem hiding this comment.
I think it is fine to call construction from a string "string constructor", but may change if I have to re-touch again for other reasons.
| BOOST_CHECK(arith_uint256S(R1L.ToString()) == R1L); | ||
| BOOST_CHECK(arith_uint256S(" 0x" + R1L.ToString() + " ") == R1L); | ||
| BOOST_CHECK(arith_uint256S("") == ZeroL); | ||
| BOOST_CHECK(R1L == arith_uint256S(R1ArrayHex)); |
There was a problem hiding this comment.
These same tests also exist in uint256_tests. What are these additionally testing now? Can they be removed?
There was a problem hiding this comment.
There are two files. arith_uint256_tests.cpp, which mostly tests the arith* class, and uint256_tests.cpp, which mostly tests the uint* class.
There was a problem hiding this comment.
They're not really testing arith_uint256 anymore though, just the new conversion helper. I don't feel strongly about this, these are just a bunch of test lines after all, but was curious what the criteria was here.
There was a problem hiding this comment.
Yeah, this line is checking the arith_uint::operator==, or maybe a test self-sanity-check? Seems fine to keep when in doubt, I guess.
|
ACK fa63f16 |
The string interface (
base_uint(const std::string&), as well asbase_uint::SetHex) is problematic for many reasons:uint256string interface:std::string -> uint256 -> UintToArith256.uint256string interface, which is brittle due to the use ofc_str()(embedded null will be treated as end-of string), etc ...Instead of fixing the interface, remove it since it is unused and redundant with
UintToArith256.