< kallewoof>
So, bitcoin core right now seems to lean towards constant func/method parameters to pointers in the form "type* const" (55 instances of "CWallet* const pwallet") over "const type*" (15 instances of "const CWallet*"), but from what I understand, "type* const" means the pointer itself is const (i.e. "pwallet = blarf" is illegal) but the pointed to instance is not const (i.e. pwallet->MutableThing() is legal). I tested this
< kallewoof>
by adding a call to pwallet->handleNotifications(), a non-const marked function, and it compiles w/o issue.
< kallewoof>
Switching the parameter to "const CWallet* pwallet" and recompiling gave an error, so yeah, those type* const declarations are all pretty much pointless.
< sipa>
kallewoof: that's correct
< sipa>
i wouldn't say it's "leaning towards"... they're completely different things
< sipa>
with different meanings
< sipa>
if someone wrote on while meaning the other, that's a bug
< sipa>
you can also have "const Type* const varname" to make both the variable amd the pointed-to object const
< kallewoof>
I can't see why "retval method(CWallet* const pwallet, ...)" would ever mean "let's not change the pwallet pointer"
< sipa>
i don't understand; that is what it means
< kallewoof>
Yeah I mean, I can't see the reason why anyone would wanna do that
< kallewoof>
The thing expires on return anyway. If you really want to a CWallet pointer to something else, just make a new one. That's better practice anyway.
< sipa>
some people like making parameters const-valued if they know the function body won't change it
< sipa>
in general
< sipa>
i don't (i don't think it adds much), but it's not unreasonable to mark everything you know to be const as const
< kallewoof>
Yeah, that's a fair point. I do think this is easy to mis(s/understand). I did, until I looked it up again for the Nth time.
< sipa>
maybe it's worth trying to make the CWallet const as well in those 55 instances
< sipa>
and see if it still compiles in some of the.
< kallewoof>
I'd say that the majority of those 55 "CWallet* const" parameters were indeed meant to be "const CWallet*", but I could be mistaken.
< kallewoof>
Yeah, I'll give it a shot
< sipa>
if so, that would be strong indication that those const markers were added in error
< luke-jr>
kallewoof: the reason is so you don't accidentally change the param
< luke-jr>
I certainly mean *const when I put *const :p
< kallewoof>
luke-jr: you mean, so you don't accidentally set e.g. pwallet to something else while inside the method? sounds like a rather obvious thing to spot, though, but safe > sorry. I think the drawback in this case is that people who add code will see a bunch of "CWallet* const" declarations and assume they mean "immutable CWallets" when they mean "immutable pointers to mutable CWallets".
< aj>
better to use "Foo&" than "Foo* const" for a class member, isn't it?
< sipa>
aj: ifyou k
< sipa>
if you know it can't be nullptr
< aj>
fair point
< sipa>
also if you store references in a class and also accept a reference type in the constructor to build it with, it may not be clear to callers that the new object's lifetime is limited to that of the argument
< sipa>
if you pass a pointer, it's generally more obvious (to me, at least) that you need to be careful
< bitcoin-git>
[bitcoin] ajtowns opened pull request #18238: net_processing: Retry notfounds with more urgency (master...202002-bump-notfound) https://github.com/bitcoin/bitcoin/pull/18238
< bitcoin-git>
[bitcoin] promag opened pull request #18239: gui: Refactor to drop client and wallet models setters (master...2020-03-drop-setmodel) https://github.com/bitcoin/bitcoin/pull/18239
< promag>
wumpus: pushed #18239 to address your comment in #18064 - if you agree with the concept then #18064 can already be merged given the existing acks
< bitcoin-git>
bitcoin/master 1ef28b4 Gregory Sanders: Make AnalyzePSBT next role calculation simple, correct
< bitcoin-git>
bitcoin/master 1f88624 Samuel Dobson: Merge #18224: Make AnalyzePSBT next role calculation simple, correct
< bitcoin-git>
[bitcoin] meshcollider merged pull request #18224: Make AnalyzePSBT next role calculation simple, correct (master...analyze_psbt_role_simple) https://github.com/bitcoin/bitcoin/pull/18224
< kallewoof>
sipa, aj, luke-jr: for the record, changing to immutable-CWallet-pointers in places where it made sense (many places did non-const stuff) resulted in 68 changes; see https://github.com/bitcoin/bitcoin/compare/master...kallewoof:2002-const-fixes ... is this something people would concept ack on a PR?
< sipa>
sounds good to me; this is the sort of change that as long as it compiles, the behavior shouldn't change
< sipa>
though in general it may lead to introducing automatic copying of objects sometimes (e.g. trying to std::move a const object will work, but generally result in a copy rather than an efficient move)
< sipa>
CWallet objects aren't copied or moved though
< kallewoof>
sipa: ohh, good point.
< bitcoin-git>
[bitcoin] kallewoof opened pull request #18241: wallet/refactor: refer to CWallet immutably when possible (master...2002-const-fixes) https://github.com/bitcoin/bitcoin/pull/18241
< jonasschnelli>
why do I get a asan heap buffer overflow detection in ParseHex (vch.push_back(n);)?
< promag>
jonasschnelli: not sure, proly due to `while (true)`?
< jonasschnelli>
promag: I don't know. I though we use the same env (clang/asan) on travis
< jonasschnelli>
*thought
< promag>
but you get am error or warning?
< kallewoof>
Tried -fsanitize=address but compiler errors on crypto/sha256_sse4.cpp:44, saying "expected relocatable expression". Should probably read up on how it works somewhere.
< fanquake>
kallewoof: you might have to pass --disable-asm to configure
< fanquake>
iirc there's a note about a similar issue in our docs
< bitcoin-git>
[bitcoin] jonasschnelli opened pull request #18242: Add BIP324 encrypted p2p transport de-/serializer (only used in tests) (master...2020/03/net_v2) https://github.com/bitcoin/bitcoin/pull/18242
< kallewoof>
fanquake: aha! thanks
< instagibbs>
would people accept a "mockfeeestimation" RPC that would let the test-writer hardcode the rates? Looking at a reported issue and without this or something like it it makes writing tests difficult
< instagibbs>
obviously only active on test-related networks, hidden blah blah
< instagibbs>
issue is with respect to bumpfee*
< provoostenator>
instagibbs: sounds good to me
< provoostenator>
Makes sense for Signet too
< instagibbs>
for now I hijacked feature_estimate_fee.py to write additional test cases :P
< wumpus>
instagibbs: if it helps increasing test coverage and it doesn't complicate the non-test code paths too much it sounds like a good idea
< bitcoin-git>
[bitcoin] Sjors opened pull request #18244: rpc: have lockUnspents also lock manually selected coins (master...2020/03/rpc_coin_locks) https://github.com/bitcoin/bitcoin/pull/18244
< bitcoin-git>
[bitcoin] instagibbs opened pull request #18245: Test some transaction creation with non-empty fee estimator (master...test_est_txn) https://github.com/bitcoin/bitcoin/pull/18245
< bitcoin-git>
[bitcoin] promag opened pull request #18246: gui: Drop connectSlotsByName usage (master...2020-03-drop-connectslotsbyName) https://github.com/bitcoin/bitcoin/pull/18246
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #18247: test: Wait for both veracks in add_p2p_connection (master...2003-qaMininodeVerackRace) https://github.com/bitcoin/bitcoin/pull/18247
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #18249: test: Bump timeouts to accomodate really slow disks (master...2003-testTimeout) https://github.com/bitcoin/bitcoin/pull/18249