Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

luke-jr constants #6961

Merged
merged 4 commits into from Nov 28, 2015
Merged

luke-jr constants #6961

merged 4 commits into from Nov 28, 2015

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Nov 6, 2015

Rebased version of #6349 (2/3).

I have dropped the changes to src/chainparamsbase.cpp to only include code that has already been reviewed as part of #6349.

strUsage += HelpMessageOpt("-bind=<addr>", _("Bind to given address and always listen on it. Use [host]:port notation for IPv6"));
strUsage += HelpMessageOpt("-connect=<ip>", _("Connect only to the specified node(s)"));
strUsage += HelpMessageOpt("-discover", _("Discover own IP addresses (default: 1 when listening and no -externalip or -proxy)"));
strUsage += HelpMessageOpt("-dns", _("Allow DNS lookups for -addnode, -seednode and -connect") + " " + _("(default: 1)"));
strUsage += HelpMessageOpt("-dns", strprintf(_("Allow DNS lookups for -addnode, -seednode and -connect (default: %u)"), fNameLookup));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Reuse translation?

@dcousens
Copy link
Contributor

dcousens commented Nov 6, 2015

utACK

extern const char * const BITCOIN_PID_FILENAME;

static const bool DEFAULT_SELFSIGNED_ROOTCERTS = false;
static const bool DEFAULT_CHOOSE_DATADIR = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ugly to have defaults for these GUI specific options in the core, they are not related to util at all.
IMO the help for GUI specific options should be moved to the GUI part of the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, plus if disable wallet is enabled, it'll possibly throw a tonne of unused variable warnings (assuming -Wnounused)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, @laanwj you meant DEFAULT_SPLASHSCREEN (and DEFAULT_CHOOSE_DATADIR?)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laanwj @dcousens So where would I put those? qt/guiconstants.h? Or ratherqt/paymentrequestplus.h, qt/intro.h ...

@maflcko maflcko force-pushed the luke-jr-const branch 4 times, most recently from 16aeda9 to b10044d Compare November 9, 2015 11:42
@maflcko
Copy link
Member Author

maflcko commented Nov 9, 2015

Rebased. (Due to the translations cleanup)

@laanwj Is it ok with the gui constants like this?

@laanwj
Copy link
Member

laanwj commented Nov 9, 2015

I don't like including a GUI header from the core. We don't do that anywhere.

I'd prefer moving the part of HelpMessage with GUI specific settings to the GUI code.

@maflcko maflcko force-pushed the luke-jr-const branch 3 times, most recently from 3ab6eb5 to 2582225 Compare November 9, 2015 18:55
@maflcko
Copy link
Member Author

maflcko commented Nov 9, 2015

I put them in HelpMessageDialog()

@laanwj
Copy link
Member

laanwj commented Nov 10, 2015

I put them in HelpMessageDialog()

That's awesome. They originally came from there and they shouldn't have been moved. Thanks!

ACK

@@ -40,7 +40,7 @@ static proxyType proxyInfo[NET_MAX];
static proxyType nameProxy;
static CCriticalSection cs_proxyInfos;
int nConnectTimeout = DEFAULT_CONNECT_TIMEOUT;
bool fNameLookup = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you change this default? ok it isn't changed it was always true

@maflcko
Copy link
Member Author

maflcko commented Nov 12, 2015

Anything holding this back?

@maflcko
Copy link
Member Author

maflcko commented Nov 26, 2015

Rebased

@sipa
Copy link
Member

sipa commented Nov 28, 2015

Concept ACK, I haven't checked the code changes in detail yet, but needs rebase (again...).

@maflcko
Copy link
Member Author

maflcko commented Nov 28, 2015

Rebased.

I haven't checked the code changes in detail yet, but needs rebase (again...).

This needs rebase approximately every 4 days. I checked the code as of #6349 and verified each rebase twice. Though, I don't think constant rebase makes this PR any better.

@@ -29,6 +29,8 @@
#include <boost/thread/exceptions.hpp>

static const bool DEFAULT_LOGTIMEMICROS = false;
static const bool DEFAULT_LOGIPS = false;
static const bool DEFAULT_LOGTIMESTAMPS = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was false.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind. The fLogTimestamps variable was initialized to false before, but the default setting was true. This makes it more consistent. Good.

@sipa
Copy link
Member

sipa commented Nov 28, 2015

ACK. Read over all the code changes, and compared the output of bitcoind and bitcoin-qt's --help output.

@sipa sipa merged commit fa41d4c into bitcoin:master Nov 28, 2015
sipa added a commit that referenced this pull request Nov 28, 2015
fa41d4c [qt] Move GUI related HelpMessage() part downstream (MarcoFalke)
faf93f3 [trivial] Reuse translation and cleanup DEFAULT_* values (MarcoFalke)
3307bdb Bugfix: Omit wallet-related options from -help when wallet is not supported (Luke Dashjr)
b966aa8 Constrain constant values to a single location in code (Luke Dashjr)
@maflcko maflcko deleted the luke-jr-const branch November 28, 2015 21:15
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Nov 30, 2015
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Dec 8, 2015
zkbot added a commit to zcash/zcash that referenced this pull request Mar 21, 2018
Misc upstream PRs

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6077
  - Second commit only (first was already applied to 0.11.X and then reverted)
- bitcoin/bitcoin#6284
- bitcoin/bitcoin#6489
- bitcoin/bitcoin#6462
- bitcoin/bitcoin#6647
- bitcoin/bitcoin#6235
- bitcoin/bitcoin#6905
- bitcoin/bitcoin#6780
  - Excluding second commit (QT) and third commit (requires bitcoin/bitcoin#6993)
- bitcoin/bitcoin#6961
  - Excluding QT parts, and a small `src/policy/policy.cpp` change which depends on a bunch of other PRs, which we'll have to remember to come back to.
- bitcoin/bitcoin#7044
- bitcoin/bitcoin#8856
- bitcoin/bitcoin#9002

Part of #2074 and #2132.
zkbot added a commit to zcash/zcash that referenced this pull request Dec 4, 2019
Misc upstream PRs

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6077
  - Second commit only (first was already applied to 0.11.X and then reverted)
- bitcoin/bitcoin#6284
- bitcoin/bitcoin#6489
- bitcoin/bitcoin#6235
- bitcoin/bitcoin#6905
- bitcoin/bitcoin#6780
  - Excluding second commit (QT) and third commit (requires bitcoin/bitcoin#6993)
- bitcoin/bitcoin#6961
  - Excluding QT parts, and a small `src/policy/policy.cpp` change which depends on a bunch of other PRs, which we'll have to remember to come back to.
- bitcoin/bitcoin#7044
- bitcoin/bitcoin#8856
- bitcoin/bitcoin#9002

Part of #2074 and #2132.
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Aug 25, 2020
7e71759 Wallet: Split main logic from InitLoadWallet into CreateWalletFromFile (random-zebra)
e585dad [Wallet] refactor wallet/init interaction (random-zebra)
49646b2 [Refactor] Nuke zwalletMain global object (random-zebra)
f5f9df9 Wallet: Let the interval-flushing thread figure out the filename (Luke Dashjr)
74445e4 [wallet] Move hardcoded file name out of log messages (random-zebra)
3f1838d [Wallet] optimize return value of InitLoadWallet() (random-zebra)
539dea4 [Wallet] move "load wallet phase" to CWallet (random-zebra)
7644318 [Wallet] move wallet help string creation to CWallet (random-zebra)
a9d69b8 [trivial] Reuse translation and cleanup DEFAULT_* values (random-zebra)
cfd007a [Bug] Omit wallet-related options from -help when wallet not supported (random-zebra)
da642e6 [Refactor] More constant default values cleanup (random-zebra)
a45275a [Refactor] Implement CBaseChainParams::BaseParams(Network) (random-zebra)
09abb98 Constrain constant values to a single location in code (random-zebra)

Pull request description:

  Adapts the following refactoring PRs:

  - bitcoin#6961 Constrain constant values to a single location in code
  - bitcoin#7576 [Wallet] move wallet help string creation to CWallet
  - bitcoin#7577 move "load wallet phase" to CWallet
  - bitcoin#7608 Move hardcoded file name out of log messages
  - bitcoin#7691 refactor wallet/init interaction
  - bitcoin#8776 Wallet refactoring leading up to multiwallet

ACKs for top commit:
  furszy:
    re ACK 7e71759
  Fuzzbawls:
    ACK 7e71759

Tree-SHA512: 5fad328b9ddf8187af97d3d5fb285d0b67e73d51ac1bc44a3d57d0af86bce8b34efaab539b7bdbc4f9a2fa575a936f83788cffcc9c6d6d04cd3e63b19d399400
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 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.

None yet

5 participants