Skip to content

luke-jr constants #6961

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.

Sign up for GitHub

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

Jump to bottom
Merged
sipa merged 4 commits into from
Nov 28, 2015
Merged

luke-jr constants #6961

sipa 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.

maflcko
maflcko reviewed Nov 6, 2015
View reviewed changes
src/init.cpp Outdated
Copy link
Member Author

@maflcko maflcko Nov 6, 2015

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

laanwj
laanwj reviewed Nov 6, 2015
View reviewed changes
src/util.h Outdated
Copy link
Member

@laanwj laanwj Nov 6, 2015

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

@dcousens dcousens Nov 7, 2015

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

@dcousens dcousens Nov 7, 2015

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

@maflcko maflcko Nov 7, 2015

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

laanwj
laanwj reviewed Nov 10, 2015
View reviewed changes
src/netbase.cpp
Copy link
Member

@laanwj laanwj Nov 10, 2015

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 maflcko force-pushed the luke-jr-const branch 3 times, most recently from 1beb3fc to cfaf3b8 Compare November 17, 2015 11:54
@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.

sipa
sipa reviewed Nov 28, 2015
View reviewed changes
src/util.h
Copy link
Member

@sipa sipa Nov 28, 2015

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

@sipa sipa Nov 28, 2015

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
@sipa
Merge pull request #6961
8d26289 
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
@sipa sipa mentioned this pull request Nov 28, 2015
Closed
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Nov 30, 2015
@luke-jr
Bugfix: Omit wallet-related options from -help when wallet is not sup…
a1c27c3 
…ported

Github-Pull: bitcoin#6961
Rebased-From: 3307bdb
@jtimon jtimon mentioned this pull request Dec 3, 2015
Closed
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Dec 8, 2015
@luke-jr
Bugfix: Omit wallet-related options from -help when wallet is not sup…
72c9ac6 
…ported

Github-Pull: bitcoin#6961
Rebased-From: 3307bdb
This was referenced May 17, 2017
Open
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Mar 21, 2018
@zkbot
Auto merge of #2390 - str4d:2132-mapargs-prep, r=<try>
f1aeaec 
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
@zkbot
Auto merge of #2390 - str4d:2132-mapargs-prep, r=str4d
868c63f 
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 random-zebra mentioned this pull request Aug 17, 2020
Merged
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Aug 25, 2020
@random-zebra
Merge #1811: [Wallet][Refactoring] wallet/init refactoring backports
e89e20e 
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.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko @dcousens @laanwj @sipa @luke-jr