| Age | Commit message (Collapse) | Author |
|---|
|
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Co-authored-by: Gloria Zhao <gloriajzhao@gmail.com>
|
|
|
|
Any identifier starting with two _, or one _ followed by a capital letter is reserved for the compiler and thus must not be used. See: https://stackoverflow.com/a/228797/7130273
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
s '__pushKV' 'pushKVEnd'
s '_EraseTx' 'EraseTxNoLock'
s '_Other' 'Other'
-END VERIFY SCRIPT-
|
|
|
|
|
|
string_view
545ff924ab6303ffabd91fdfc4f0a4962daf133c refactor: use string_view for RPC named argument values (stickies-v)
7727603e44f8f674e0fc8389e78047e2b56e6052 refactor: reduce unnecessary complexity in ParseNonRFCJSONValue (stickies-v)
1d02e599012721549d4c20b1b37fcc5ee7b961b6 test: add cases to JSON parsing (stickies-v)
Pull request description:
Inspired by MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/26506#discussion_r1036149426). Main purpose of this PR is to minimize copying (potentially large) RPC named arguments when calling `.substr()` by using `std::string_view` instead of `std::string`. Furthermore, cleans up the code by removing unnecessary complexity in `ParseNonRFCJSONValue()` (done first to avoid refactoring required to concatenate `string` and `string_view`), updates some naming and adds a few test cases. Should not introduce any behaviour change.
## Questions
- ~Was there actually any merit to `ParseNonRFCJSONValue()` surrounding the value with brackets and then parsing it as an array? I don't see it, and the new approach doesn't fail any tests. Still a bit suspicious about it though.~
- Cleared up by https://github.com/bitcoin/bitcoin/pull/26506#pullrequestreview-1211984059
- If there are no objections to 7727603e44f8f674e0fc8389e78047e2b56e6052, I think we should follow up with a PR to rename `ParseNonRFCJSONValue()` to a local `Parse()` helper function (that throws if invalid), remove it from `client.h` and merge the test coverage we currently have on `ParseNonRFCJSONValue()` with the coverage we have on `UniValue::read()`.
ACKs for top commit:
ryanofsky:
Code review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c
MarcoFalke:
review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c 📻
Tree-SHA512: b1c89fb010ac9c3054b023cac1acbba2a539a09cf39a7baffbd7f7571ee268d5a6d98701c7ac10d68a814526e8fd0fe96ac1d1fb072f272033e415b753f64a5c
|
|
See https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html
|
|
Minimize copying RPC named argument values when calling .substr() by
using std::string_view instead of std::string.
|
|
Drop UniValue::getBool method because it is easy to confuse with the
UniValue::get_bool method, and could potentially cause bugs. Unlike get_bool,
getBool doesn't ensure that the value is a boolean and returns false for all
integer, string, array, and object values instead of throwing an exceptions.
The getBool method is also redundant because it is an alias for isTrue. There
were only 5 getBool() calls in the codebase, so this commit replaces them with
isTrue() or get_bool() calls as appropriate.
These changes were originally made by MarcoFalke in
https://github.com/bitcoin/bitcoin/pull/26213 but were dropped to limit the
scope of that PR.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
|
|
fa095257511e53d7a593c6714724aafb484e6b6f univalue: string_view test (MacroFake)
1111c7e3f1f5c550f62016d71ccc518aafd97acf univalue: Avoid std::string copies (MacroFake)
Pull request description:
This shouldn't matter too much, unless a really large string is pushed into a json struct, but I think it also clarifies the code.
ACKs for top commit:
martinus:
Code review ACK https://github.com/bitcoin/bitcoin/commit/fa095257511e53d7a593c6714724aafb484e6b6f
aureleoules:
reACK fa095257511e53d7a593c6714724aafb484e6b6f
ryanofsky:
Code review ACK fa095257511e53d7a593c6714724aafb484e6b6f
Tree-SHA512: 74c441912bd0b00cdb9ea7890121f71ae5d62a7594e7d29aa402c9e3f033710c5d3afb27a37c552e6513804b249aa37e375ce013a3db853a25d1fd7b6e6cd3a8
|
|
By throwing a custom exception from `Univalue::checkType` (instead of a plain
std::runtime_error) and catching it on the RPC server request handler.
So we properly return RPC_TYPE_ERROR (-3) on arg type errors and
not the general RPC_MISC_ERROR (-1).
|
|
|
|
|
|
|
|
integral type confusions
fa23c197509f692a815193acc1b50bad2fcbedfe univalue: Avoid narrowing and verbose int constructors (MacroFake)
fa3a9a1e8d9b6dffda772e97c279f3c0af6813f9 rpc: Select int-UniValue constructor for enum value in upgradewallet RPC (MacroFake)
Pull request description:
As UniValue provides several constructors for integral types, the
compiler is unable to select one if the passed type does not exactly
match. This is unintuitive for developers and forces them to write
verbose and brittle code. (Refer to `-Wnarrowing` compiler warning)
For example, there are many places where an unsigned int is cast to a
signed int. While the cast is safe in practice, it is still needlessly
verbose and confusing as the value can never be negative. In fact it
might even be unsafe if the unsigned value is large enough to map to a
negative signed one.
Fix this issue and other (minor) type issues.
ACKs for top commit:
aureleoules:
ACK fa23c197509f692a815193acc1b50bad2fcbedfe.
Tree-SHA512: 7d99b5b90c7d8eed2e3448167255a59e817dd6b8fcfc1b17c69ddefd0db33d1bf4344fbcd8b7f8685b58182c0f572ab9ffa99467afa666ac21843df7ea645033
|
|
|
|
Can be reviewed with the git options
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
|
|
As UniValue provides several constructors for integral types, the
compiler is unable to select one if the passed type does not exactly
match. This is unintuitive for developers and forces them to write
verbose and brittle code.
For example, there are many places where an unsigned int is cast to a
signed int. While the cast is safe in practice, it is still needlessly
verbose and confusing as the value can never be negative. In fact it
might even be unsafe if the unsigned value is large enough to map to a
negative signed one.
|
|
|
|
|
|
Mostly changes to remove src/univalue exceptions from the various linters,
and the required code changes to make them happy. As well as minor doc
changes.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
This merge commit bumps the univalue subtree and also updates the whitespace
for some failing tests.
|
|
|
|
|
|
|
|
|
