Constrain constant values to a single location in code #6349
Conversation
|
No wallet build fails with: CXX libbitcoin_server_a-init.o |
|
concept ACK.
|
|
concept ACK |
|
@jgarzik I put it in rpcserver.h because the -rest option is used in rpcserver.cpp, which handles all HTTP requests. There is no rest.h - do you really want me to make one just for this? |
|
@paveljanik Fixed. |
src/main.h
Outdated
There was a problem hiding this comment.
Why did you call this TXINDEX_DEFAULT instead of DEFAULT_TXINDEX?
There was a problem hiding this comment.
Dunno, changed for consistency.
src/util.h
Outdated
There was a problem hiding this comment.
Declaring the constant and its value in a header won't work well for data structures that require a larger amount of memory or indirected pointers, such as strings, structs, objects etc. In worst case this causes errors in some compilers, in best case this causes unnecessary memory use.
There was a problem hiding this comment.
So please move the content of the variable and BITCOIN_PID_FILENAME to util.cpp
Apart from that I'd like to merge this
|
Concept ACK |
|
Needs rebase + string nit addresed |
|
concept ACK |
|
concept ACK - as @laanwj says it appears merge-ready once rebased + nits addressed |
…database, not memory pool)
…t; also show correct default in getmininginfo
…as compiled on Also hides the option for non-GUI builds.
|
Rebased, addressed nit, added equivalent commit for -uiplatform. (Travis passes.) |
There was a problem hiding this comment.
I don't think we should use uiInterface here, nor put this uiplatform id as a variable on the UI interface (as bitcoind has no business with it). I'd just check for mode == HMM_BITCOIN_QT, like for the other options.
There was a problem hiding this comment.
Hmm I understand now, you want to show the actual default here. Ok then it makes sense.
We should never have moved the UI-specific options back to init, but that's not your fault.
|
utACK, however I didn't cross verify the constants. |
|
utACK 29266b6. |
|
Does this need more reviewers or can it be merged as is? (Any further merge conflicts would only retrigger review, so let's try to finish with this PR.) |
|
Needs rebase (caused by #6235) |
|
Fixed by #6961. |
9 participants
No description provided.