Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Oct 30, 2019

MarcoFalke added 2 commits October 30, 2019 16:22
5a58a46671 Merge #21: Remove hand-coded UniValue destructor.
b4cdfc4f47 Remove hand-coded UniValue destructor.
7fba60b5ad Merge #17: [docs] Update readme
4577454e7e Merge #13: Fix typo
ac7e73cda8 [docs] Update readme
4a4964729b Fix typo

git-subtree-dir: src/univalue
git-subtree-split: 5a58a46671d3c1711e93d2fc7961085515c8c7a7
@maflcko
Copy link
Member Author

maflcko commented Oct 30, 2019

The last update was more than a year ago: #14164 (comment)

@laanwj
Copy link
Member

laanwj commented Oct 31, 2019

There have also been some changes last year in https://github.com/jgarzik/univalue/commits/master, do we want to include them?

@jnewbery
Copy link
Contributor

ACK fa439e8

@maflcko
Copy link
Member Author

maflcko commented Oct 31, 2019

There have also been some changes last year in https://github.com/jgarzik/univalue/commits/master, do we want to include them?

We check the return value of Univalue::read everywhere, so I'd rather not add some clumsy goto. I'd prefer a nodiscard attribute.

@jgarzik
Copy link
Contributor

jgarzik commented Oct 31, 2019

There have also been some changes last year in https://github.com/jgarzik/univalue/commits/master, do we want to include them?

We check the return value of Univalue::read everywhere, so I'd rather not add some clumsy goto. I'd prefer a nodiscard attribute.

Can you specify which commit or line of code this refers to? Bitcoin Core is a major user of univalue, and we want to avoid a situation where Bitcoin's univalue diverges from the Debian/Ubuntu univalue.

@DrahtBot
Copy link
Contributor

Bench

microbenches

Bench config

Bitcoinperf commit e671f5d5dfd49724af3b234ebfd1076345822e92
OS: Ubuntu Eoan
YML:

---

# Cache a built bitcoin src directory and restore it from the cache on
# subsequent runs.
cache_build: false

# If true, the first git clone will be cached and copied from as necessary.
cache_git: true

# Set to false to make cache dropping optional and bypass various safety checks.
safety_checks: false

compilers:
  - clang
  - gcc

benches:
  build:
    num_jobs: 9

  microbench:
    filter: '.*Json.*'

to_bench:

  - gitref: 'fa439e88af944082875e1fdb1cd8bb5a74b88b56'

  - gitref: 'master'

@DrahtBot
Copy link
Contributor

Gitian builds

File commit 08e2947
(master)
commit b6dee2867e03b1b6d5c3a5524bdaf0d383e23cee
(master and this pull)
bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz 00b1465b4199e5a9... bd87017df20f4796...
bitcoin-0.19.99-aarch64-linux-gnu.tar.gz 665c2633693785b7... 3d2d9486683ca03d...
bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz 0be5c1b77ba6a66b... 78e643a2be2a084d...
bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz 370d8b18ae0f6026... 5d9427fc837232de...
bitcoin-0.19.99-i686-pc-linux-gnu-debug.tar.gz 60c9df005c733991... 8626c48fc8ed679b...
bitcoin-0.19.99-i686-pc-linux-gnu.tar.gz b05dc039b6b4447a... 3f6a0f4875b7b3db...
bitcoin-0.19.99-osx-unsigned.dmg 404f1195d6b57945... 17fcc2a92d408664...
bitcoin-0.19.99-osx64.tar.gz 2ce2ee4b9c3d4164... a464d3f85207c551...
bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz b7050d25c59992e4... 95513a7ca0a845fa...
bitcoin-0.19.99-riscv64-linux-gnu.tar.gz d172699be8e0c0d1... f24a9012c703589b...
bitcoin-0.19.99-win64-debug.zip 1f3d5167443ff984... cb4d1a849bcb3443...
bitcoin-0.19.99-win64-setup-unsigned.exe 44fb74650e3d85d4... 580add514d677004...
bitcoin-0.19.99-win64.zip 38942c65e9ad0810... d5038709bc1dc49f...
bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz 704eee6bfa940e63... 27e27bfc9f3b5e4a...
bitcoin-0.19.99-x86_64-linux-gnu.tar.gz dfca3e31b40e1fe0... d05b65b0a3e7caf6...
bitcoin-0.19.99.tar.gz cf0fc2fd3d592a12... aba4774e53fb08a4...
bitcoin-core-linux-0.20-res.yml 20249e5e68aa0c1b... b097a933aa2c962b...
bitcoin-core-osx-0.20-res.yml 3cd30f76c232b8ac... 55e589689bc5588d...
bitcoin-core-win-0.20-res.yml a4349a184bc08402... 2da53c846f3b4e08...
linux-build.log 0183d87a737bfd87... f27b3c485e3c6fa7...
osx-build.log 0f01eb2509cb12c3... efcaadd0ca459548...
win-build.log ea9197760a17c9d2... 339ef1a3b861d972...
bitcoin-core-linux-0.20-res.yml.diff b3b3a5c5a1c425fa...
bitcoin-core-osx-0.20-res.yml.diff 2ae5cf9ab2e0db5e...
bitcoin-core-win-0.20-res.yml.diff 122bfab20dff08b0...
linux-build.log.diff fbff79b3de64ca4d...
osx-build.log.diff 54b98f5ecad88339...
win-build.log.diff 153ee20f65a44863...

@laanwj
Copy link
Member

laanwj commented Nov 1, 2019

We check the return value of Univalue::read everywhere, so I'd rather not add some clumsy goto. I'd prefer a nodiscard attribute.

Ok. Thanks for the explanation It's useful to document why we're not taking certain changes from upstream.

ACK fa439e8

@jgarzik
Copy link
Contributor

jgarzik commented Nov 1, 2019

We check the return value of Univalue::read everywhere, so I'd rather not add some clumsy goto. I'd prefer a nodiscard attribute.

Ok. Thanks for the explanation It's useful to document why we're not taking certain changes from upstream.

It would be better for the trees not to diverge. Will look at removing "clumsy goto" related code, and work to eliminate this code-divergence issue.

maflcko pushed a commit that referenced this pull request Nov 1, 2019
fa0b3da Squashed 'src/univalue/' changes from 7890db99d6..5a58a46671 (MarcoFalke)

Pull request description:

  Only change is a performance improvement. See bitcoin-core/univalue-subtree#21 (comment) and bitcoin-core/univalue-subtree#15 (comment)

ACKs for top commit:
  jnewbery:
    ACK fa439e8
  laanwj:
    ACK fa439e8

Tree-SHA512: 35ea8f76ea4806182949c8eb5a8b652d1aaeec03ee023838e7cb29abcb81c61d59b38f15708564a78574722df57d13f61ef17d0f670a4056819705ef892321e0
@maflcko maflcko merged commit fa439e8 into bitcoin:master Nov 1, 2019
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 1, 2019
fa0b3da Squashed 'src/univalue/' changes from 7890db99d6..5a58a46671 (MarcoFalke)

Pull request description:

  Only change is a performance improvement. See bitcoin-core/univalue-subtree#21 (comment) and bitcoin-core/univalue-subtree#15 (comment)

ACKs for top commit:
  jnewbery:
    ACK fa439e8
  laanwj:
    ACK fa439e8

Tree-SHA512: 35ea8f76ea4806182949c8eb5a8b652d1aaeec03ee023838e7cb29abcb81c61d59b38f15708564a78574722df57d13f61ef17d0f670a4056819705ef892321e0
@luke-jr
Copy link
Member

luke-jr commented Nov 5, 2019

The so-called "clumsy goto" in this case looks like pretty reasonable use of goto...

@maflcko maflcko deleted the 1910-updateUnivalue branch November 5, 2019 18:25
@maflcko
Copy link
Member Author

maflcko commented Nov 5, 2019

We don't use goto anywhere else in the code base and this case can be replaced with a one line helper function that clears the Univalue and returns false.

@laanwj
Copy link
Member

laanwj commented Nov 5, 2019

Also, while it has definite uses in C, the use of goto is generally unnecessary in modern C++.

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 14, 2020
fa0b3da Squashed 'src/univalue/' changes from 7890db99d6..5a58a46671 (MarcoFalke)

Pull request description:

  Only change is a performance improvement. See bitcoin-core/univalue-subtree#21 (comment) and bitcoin-core/univalue-subtree#15 (comment)

ACKs for top commit:
  jnewbery:
    ACK fa439e8
  laanwj:
    ACK fa439e8

Tree-SHA512: 35ea8f76ea4806182949c8eb5a8b652d1aaeec03ee023838e7cb29abcb81c61d59b38f15708564a78574722df57d13f61ef17d0f670a4056819705ef892321e0
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
fa0b3da Squashed 'src/univalue/' changes from 7890db99d6..5a58a46671 (MarcoFalke)

Pull request description:

  Only change is a performance improvement. See bitcoin-core/univalue-subtree#21 (comment) and bitcoin-core/univalue-subtree#15 (comment)

ACKs for top commit:
  jnewbery:
    ACK fa439e8
  laanwj:
    ACK fa439e8

Tree-SHA512: 35ea8f76ea4806182949c8eb5a8b652d1aaeec03ee023838e7cb29abcb81c61d59b38f15708564a78574722df57d13f61ef17d0f670a4056819705ef892321e0
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants