< bitcoin-git>
[bitcoin] Csi18nAlistairMann opened pull request #15387: docs: add "sections" info to example bitcoin.conf (master...confSections) https://github.com/bitcoin/bitcoin/pull/15387
< wumpus>
do we have any code-wide preference for the order of input versus output arguments?
< wumpus>
I don't *think* so, but we probably should
< wumpus>
(this is something I realized while reviewing #13932)
< provoostenator>
wumpus: I also noticed it. Initially I was confused, but then found some reasonable arguments in favor of putting outputs first, because the number of input arguments tends to grow over time.
< provoostenator>
But I don't think it's worth making rules around that.
< wumpus>
at the least, I think having methods with different orderings on the same class is confusing
< gmaxwell>
outputs first looks like assignment, output = input. :)
< wumpus>
I don't have any particular preference I just like consistency
< sipa>
libsecp256k1 has a strict ordering of output first, then inputs, and within each ordered from most-recently-computed to oldest
< provoostenator>
Although in general I often find it hard to tell when I see methods get called. It's more clear in the method definitions.
< sipa>
but imposing such rules on an existing project sounds much more burden than it's worth
< wumpus>
would be better to simply return output arguments, too bad we can't use std::optional
< provoostenator>
Especially with all the references it gets confusing fast.
< wumpus>
sipa: I agree but what about new code?
< gmaxwell>
(also it's a common convention in libc, e.g. memcpy(dest,src,...))
< provoostenator>
std::optional sounds great. Part of c++17 so looking forward to using that in 2023
< sipa>
c++ stl usually has inputs first, outputs last
< sipa>
(different from libc)
< wumpus>
yes
< gmaxwell>
(sprintf(buf, fmt, vaargs)...)
< wumpus>
I don't think c++'s reference syntax made output arguments particularly clearer
< wumpus>
at least in C it's usually recognizable because the caller has to take a pointer
< gmaxwell>
(to be clear, I don't particularly care, I have a preference for dest first in C, but C++ is already crazy regardless of the order. :P )
< sipa>
google c++ only lets you pass const references; if you want something mutable, it needs to be a pointer
< wumpus>
in C++ it's sometimes magic 'oh, so this value gets changed'
< wumpus>
sipa: I like that
< sipa>
(which makes sense syntactically, but semantically is somewhat ridiculous... modern c++ shouldn't need to deal with raw pointers pretty much ever)
< sipa>
that was also pre-c++11, i don't know what changed
< sipa>
ideally output arguments are actual outputs, and with std::pair/tuple and move semantics that became a lot more reasonable
< wumpus>
yea, I don't mean we should do that, but I like that some people thought about this and came to a similar conclusion as me
< sipa>
c++17 structured bindings are even nicer
< sipa>
auto [a,b,c] = function_returning_3tuple()
< wumpus>
yes, nice
< * sipa>
patiently awaits RHEL to adopt GCC 8
< wumpus>
ideally "output" arguments would only be necessary for "inout" arguments that get modified in place
< wumpus>
not for anything that is created anew
< wumpus>
GCC 8 ? that's going to be a long wait I'm afraid
< sipa>
most of it is in gcc 7
< wumpus>
gcc 7 is in ubuntu 18.04 LTS that's good at least
< bitcoin-git>
[bitcoin] laanwj closed pull request #15155: test: Support -cli tests using external bitcoin-cli (master...test_external_bcli) https://github.com/bitcoin/bitcoin/pull/15155
< rafalcpp>
probably not a concern since we run known code inside Gitian LXC container
< wumpus>
not a concern for gitian but still good to know, thanks
< rafalcpp>
unless, someone would trick a person to build for him his PR in Gitian just to test things, and take over account of e.g. a developer
< rafalcpp>
wumpus: Gitian is known to not be affected by this?
< wumpus>
I don't think the gitian system is otherwise hardened against, say, malicious descriptors
< wumpus>
so I don't think a possible escape changes much
< wumpus>
but anyhow, upgrade if you can
< rafalcpp>
before distros are fully update, it is probably good if main developers would run Gitian on separate machine, and sign results on another. Sorry to be pedantic, but it's BTC... :)
< wumpus>
FWIW I've always done that
< luke-jr>
rafalcpp: it's a concern for anyone who does gitian builds of arbitrary repos
< luke-jr>
and yes, my gitian sign script is `true` :P
< wumpus>
right, that ^^ if anyone would be able to sneak a VM escape into the gitian descriptors in one of our main branches things would be really bad
< bitcoin-git>
[bitcoin] Sjors opened pull request #15388: [build] Makefile.am: add rule for src/bitcoin-wallet (master...2019/02/wallet_tool_make) https://github.com/bitcoin/bitcoin/pull/15388
< wumpus>
this is really weird code too "(strProxy == strDefaultProxyGUI.toStdString()) ? ui->proxyReachIPv4->setChecked(true) : ui->proxyReachIPv4->setChecked(false);" I've never seen a tertiary operator used to switch between imperative statements
< wumpus>
(which return void)
< wumpus>
especially as they could have simply used ui->proxyReachIPv4->setChecked(strProxy == strDefaultProxyGUI.toStdString()) ?
< luke-jr>
wumpus: IIRC, it was contributed by a new dev at the time
< luke-jr>
so I wouldn't assume there are good reasons
< bitcoin-git>
[bitcoin] practicalswift opened pull request #15391: Add compile time verification of assumptions we're currently making implicitly/tacitly (master...assumptions) https://github.com/bitcoin/bitcoin/pull/15391
< promag>
could we just enable #13339 for non-win builds for 0.18?
< provoostenator>
promag: the author of cpp-subprocess said he found boost::process implementation unpretty. I haven't studied it in enough detail yet to have a strong opinion. But aren't we trying to nuke Boost?
< luke-jr>
provoostenator: we're trying to migrate from boost to C++11; if C++11 doesn't provide it, no reason to avoid boost for it
< luke-jr>
(I'm not sure there's much left to migrate at this point either?)
< provoostenator>
Ok, sounds like I'll have to study both implementations then :-)
< provoostenator>
Assuming Boost does work with Windows that would be a good argument for it.
< jnewbery>
Great to see #15153 merged. I think it could do with a bit more review (I plan to when I get a chance)
< luke-jr>
has there been any discussion previously about the binary name for bitcoin-wallet? seems strange to me.
< promag>
provoostenator: what luke-jr said. anyway, I don't have experience with boost::process, just think it's nice by reading the docs.
< provoostenator>
luke-jr jnewbery: I was thoroughly confused by this naming today while debugging the Makefile, because that actualy contains "bitcoin-wallet" as an intermediate stage for the wallet itself.
< provoostenator>
Might be better to call it wallet-tool
< provoostenator>
So wallet-tool makes sense for this tool. Or even offline-wallet or something. The live wallet in a split code base, which connects to a node, could then be called bitcoin-wallet.
< jnewbery>
ryanofsky: thoughts ^ ?
< provoostenator>
Ironically bitcoin-wallet was ryanofsky's idea in that comment :-)
< jnewbery>
I currently have no strong opinion one way or the other (but think we should make a decision before 0.18 freeze)
< ryanofsky>
i was just thinking that you have one binary called bitcoin-wallet that you use for all wallet stuff
< ryanofsky>
not sure what advantage there would be to having separate bitcoin-wallet and bitcoin-wallet-tool binaries
< jnewbery>
That seems like a reasonable suggestion to me. Merge wallet tool functionality into the new new bitcoin-wallet when it happens
< ryanofsky>
yeah, i already implemented that (though can't remember if i pushed it to 10102 yet)
< provoostenator>
Any volunteers for bumping QT to 5.5 as per #13478?
< jnewbery>
so for now, keep the wallet tool as bitcoin-wallet, and when the separated bitcoin-wallet happens the tool commands will continue to work as expected?
< provoostenator>
ryanofsky: that works for me as well. If the Makefile confuses someone again, we can also rename the intermediate bitcoin_wallet step.
< gmaxwell>
FWIW, I actually misread PRs related to the wallet tool and thought it was named bitcoin-wallet-tool
< provoostenator>
I think appveyor is on strike by the way.
< provoostenator>
gmaxwell: it was renamed from bitcoin-wallet-tool to bitcoin-wallet along the way.
< bitcoin-git>
bitcoin/master 8c9b8a3 Hennadii Stepanov: Replace script name with special parameter
< bitcoin-git>
bitcoin/master d739184 MarcoFalke: Merge #15216: Scripts and tools: Replace script name with a special parame...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #15216: Scripts and tools: Replace script name with a special parameter (master...20190120-improve-shell-ux) https://github.com/bitcoin/bitcoin/pull/15216