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.

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

qt, refactor: Remove never used default parameter #17943

Merged
merged 2 commits into from
Jan 17, 2020
Merged

qt, refactor: Remove never used default parameter #17943

merged 2 commits into from
Jan 17, 2020

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jan 16, 2020

In BitcoinGUI::message() slot the bool* ret = nullptr parameter is never used.

This PR removes it and simplifies connections syntax by replacing lambdas with the &BitcoinGUI::message slot.

@promag
Copy link
Contributor

promag commented Jan 16, 2020

Concept ACK, when was the last usage dropped?

@DrahtBot DrahtBot added the GUI label Jan 16, 2020
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 16, 2020 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16224 (gui: Bilingual GUI error messages by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@Empact
Copy link
Contributor

Empact commented Jan 16, 2020 •
edited
Loading

The use was introduced in f7f3a96 and I believe removed in 35ecf85 but haven't built the old code to confirm.

@Empact
Copy link
Contributor

Empact commented Jan 16, 2020

Code review ACK 1a53b0d

@promag promag mentioned this pull request Jan 17, 2020
Merged
@promag
Copy link
Contributor

promag commented Jan 17, 2020

Code review ACK 1a53b0d.

@Sjors
Copy link
Member

Sjors commented Jan 17, 2020 •
edited
Loading

Tested ACK 1a53b0d

You can see messages in action by e.g. feeding a malformed BIP20 URI: src/qt/bitcoin-qt -regtest bitcoin://bcrt1q

fanquake added a commit that referenced this pull request Jan 17, 2020
@fanquake
Merge #17943: qt, refactor: Remove never used default parameter
0deba68 
1a53b0d refactor: Simplify connection syntax (Hennadii Stepanov)
7d0a8f4 refactor: Remove never used default parameter (Hennadii Stepanov)

Pull request description:

  In `BitcoinGUI::message()` slot the `bool* ret = nullptr` parameter is never used.

  This PR removes it and simplifies connections syntax by replacing lambdas with the `&BitcoinGUI::message` slot.

ACKs for top commit:
  promag:
    Code review ACK 1a53b0d.
  Sjors:
    Tested ACK 1a53b0d
  Empact:
    Code review ACK 1a53b0d

Tree-SHA512: e287c3218d31a387338d50da3de79c27e8691829449c3a75a2f75bb1c680bd81eb9de43e4dd3646560a422d4a45c84debfce9783c4376b50aa5cde491f300688
@fanquake fanquake merged commit 1a53b0d into bitcoin:master Jan 17, 2020
@hebasto hebasto deleted the 20200116-message-parameter branch January 18, 2020 09:14
@hebasto hebasto mentioned this pull request Jan 18, 2020
Merged
@hebasto
Copy link
Member Author

hebasto commented Jan 18, 2020 •
edited
Loading

Sorry, It seems I broke ThreadSafeMessageBox() ((

bool* ret actually is used here:

bitcoin/src/qt/bitcoingui.cpp

Line 1368 in a654626

Q_ARG(bool*, &ret));

How it could be reverted?

Ref: #16348

@promag
Copy link
Contributor

promag commented Jan 18, 2020

Please don't revert, I think the code you referenced is broken.

@hebasto
Copy link
Member Author

hebasto commented Jan 19, 2020

@fanquake (from IRC):

It's unclear from your comments what is supposedly broken or not.

$ ./src/qt/bitcoin-qt 
: You need to rebuild the database using -reindex to go back to unpruned mode.  This will redownload the entire blockchain.
Please restart with -reindex or -reindex-chainstate to recover.
bitcoin-qt: qt/bitcoingui.cpp:1369: bool ThreadSafeMessageBox(BitcoinGUI*, const string&, const string&, unsigned int): Assertion `invoked' failed.
Aborted (core dumped)

@hebasto hebasto mentioned this pull request Jan 19, 2020
Merged
laanwj added a commit that referenced this pull request Jan 22, 2020
@laanwj
Merge #17965: qt: Revert changes of pr17943
0038e53 
70e4706 Revert "refactor: Remove never used default parameter" (Hennadii Stepanov)
219417b Revert "refactor: Simplify connection syntax" (Hennadii Stepanov)

Pull request description:

  The code, the `bool* ret = nullptr` parameter in the `BitcoinGUI::message()` slot, removed in #17943 is not dead actually. It is used in `ThreadSafeMessageBox()` function:
  https://github.com/bitcoin/bitcoin/blob/a654626f076a72416a3d354218d7107571d6caaf/src/qt/bitcoingui.cpp#L1363-L1368

  Now in master (a654626):
  ```
  $ ./src/qt/bitcoin-qt -prune=-1
  Error: Prune cannot be configured with a negative value.
  bitcoin-qt: qt/bitcoingui.cpp:1369: bool ThreadSafeMessageBox(BitcoinGUI*, const string&, const string&, unsigned int): Assertion `invoked' failed.
  Aborted (core dumped)
  ```

  This PR reverts all commits of #17943

  Additional notes: the bug was missed due to dynamic function call `QMetaObject::invokeMethod()` which cannot be checked at compile time. See #16348 for more discussion.

  Sorry for introducing a bug.

ACKs for top commit:
  Sjors:
    ACK 70e4706
  laanwj:
    ACK 70e4706

Tree-SHA512: b968a026eaa4f5f39fd36ddc715d8e233f3c6420e6580f11d4ca422a5ff5d1d9d3df9ac11b353c3d4f434d67d6a69e37d2e26b8248d72bedd14ecba0a545a327
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 22, 2021
@fanquake @PastaPastaPasta
Merge bitcoin#17943: qt, refactor: Remove never used default parameter
10185ac 
1a53b0d refactor: Simplify connection syntax (Hennadii Stepanov)
7d0a8f4 refactor: Remove never used default parameter (Hennadii Stepanov)

Pull request description:

  In `BitcoinGUI::message()` slot the `bool* ret = nullptr` parameter is never used.

  This PR removes it and simplifies connections syntax by replacing lambdas with the `&BitcoinGUI::message` slot.

ACKs for top commit:
  promag:
    Code review ACK 1a53b0d.
  Sjors:
    Tested ACK 1a53b0d
  Empact:
    Code review ACK bitcoin@1a53b0d

Tree-SHA512: e287c3218d31a387338d50da3de79c27e8691829449c3a75a2f75bb1c680bd81eb9de43e4dd3646560a422d4a45c84debfce9783c4376b50aa5cde491f300688
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 22, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#17965: qt: Revert changes of pr17943
8af1aba 
70e4706 Revert "refactor: Remove never used default parameter" (Hennadii Stepanov)
219417b Revert "refactor: Simplify connection syntax" (Hennadii Stepanov)

Pull request description:

  The code, the `bool* ret = nullptr` parameter in the `BitcoinGUI::message()` slot, removed in bitcoin#17943 is not dead actually. It is used in `ThreadSafeMessageBox()` function:
  https://github.com/bitcoin/bitcoin/blob/a654626f076a72416a3d354218d7107571d6caaf/src/qt/bitcoingui.cpp#L1363-L1368

  Now in master (a654626):
  ```
  $ ./src/qt/bitcoin-qt -prune=-1
  Error: Prune cannot be configured with a negative value.
  bitcoin-qt: qt/bitcoingui.cpp:1369: bool ThreadSafeMessageBox(BitcoinGUI*, const string&, const string&, unsigned int): Assertion `invoked' failed.
  Aborted (core dumped)
  ```

  This PR reverts all commits of bitcoin#17943

  Additional notes: the bug was missed due to dynamic function call `QMetaObject::invokeMethod()` which cannot be checked at compile time. See bitcoin#16348 for more discussion.

  Sorry for introducing a bug.

ACKs for top commit:
  Sjors:
    ACK 70e4706
  laanwj:
    ACK 70e4706

Tree-SHA512: b968a026eaa4f5f39fd36ddc715d8e233f3c6420e6580f11d4ca422a5ff5d1d9d3df9ac11b353c3d4f434d67d6a69e37d2e26b8248d72bedd14ecba0a545a327
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 22, 2021
@fanquake @PastaPastaPasta
Merge bitcoin#17943: qt, refactor: Remove never used default parameter
0a10713 
1a53b0d refactor: Simplify connection syntax (Hennadii Stepanov)
7d0a8f4 refactor: Remove never used default parameter (Hennadii Stepanov)

Pull request description:

  In `BitcoinGUI::message()` slot the `bool* ret = nullptr` parameter is never used.

  This PR removes it and simplifies connections syntax by replacing lambdas with the `&BitcoinGUI::message` slot.

ACKs for top commit:
  promag:
    Code review ACK 1a53b0d.
  Sjors:
    Tested ACK 1a53b0d
  Empact:
    Code review ACK bitcoin@1a53b0d

Tree-SHA512: e287c3218d31a387338d50da3de79c27e8691829449c3a75a2f75bb1c680bd81eb9de43e4dd3646560a422d4a45c84debfce9783c4376b50aa5cde491f300688
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 22, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#17965: qt: Revert changes of pr17943
70c48b9 
70e4706 Revert "refactor: Remove never used default parameter" (Hennadii Stepanov)
219417b Revert "refactor: Simplify connection syntax" (Hennadii Stepanov)

Pull request description:

  The code, the `bool* ret = nullptr` parameter in the `BitcoinGUI::message()` slot, removed in bitcoin#17943 is not dead actually. It is used in `ThreadSafeMessageBox()` function:
  https://github.com/bitcoin/bitcoin/blob/a654626f076a72416a3d354218d7107571d6caaf/src/qt/bitcoingui.cpp#L1363-L1368

  Now in master (a654626):
  ```
  $ ./src/qt/bitcoin-qt -prune=-1
  Error: Prune cannot be configured with a negative value.
  bitcoin-qt: qt/bitcoingui.cpp:1369: bool ThreadSafeMessageBox(BitcoinGUI*, const string&, const string&, unsigned int): Assertion `invoked' failed.
  Aborted (core dumped)
  ```

  This PR reverts all commits of bitcoin#17943

  Additional notes: the bug was missed due to dynamic function call `QMetaObject::invokeMethod()` which cannot be checked at compile time. See bitcoin#16348 for more discussion.

  Sorry for introducing a bug.

ACKs for top commit:
  Sjors:
    ACK 70e4706
  laanwj:
    ACK 70e4706

Tree-SHA512: b968a026eaa4f5f39fd36ddc715d8e233f3c6420e6580f11d4ca422a5ff5d1d9d3df9ac11b353c3d4f434d67d6a69e37d2e26b8248d72bedd14ecba0a545a327
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 22, 2021
@fanquake @PastaPastaPasta
Merge bitcoin#17943: qt, refactor: Remove never used default parameter
b614776 
1a53b0d refactor: Simplify connection syntax (Hennadii Stepanov)
7d0a8f4 refactor: Remove never used default parameter (Hennadii Stepanov)

Pull request description:

  In `BitcoinGUI::message()` slot the `bool* ret = nullptr` parameter is never used.

  This PR removes it and simplifies connections syntax by replacing lambdas with the `&BitcoinGUI::message` slot.

ACKs for top commit:
  promag:
    Code review ACK 1a53b0d.
  Sjors:
    Tested ACK 1a53b0d
  Empact:
    Code review ACK bitcoin@1a53b0d

Tree-SHA512: e287c3218d31a387338d50da3de79c27e8691829449c3a75a2f75bb1c680bd81eb9de43e4dd3646560a422d4a45c84debfce9783c4376b50aa5cde491f300688
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 22, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#17965: qt: Revert changes of pr17943
3e28512 
70e4706 Revert "refactor: Remove never used default parameter" (Hennadii Stepanov)
219417b Revert "refactor: Simplify connection syntax" (Hennadii Stepanov)

Pull request description:

  The code, the `bool* ret = nullptr` parameter in the `BitcoinGUI::message()` slot, removed in bitcoin#17943 is not dead actually. It is used in `ThreadSafeMessageBox()` function:
  https://github.com/bitcoin/bitcoin/blob/a654626f076a72416a3d354218d7107571d6caaf/src/qt/bitcoingui.cpp#L1363-L1368

  Now in master (a654626):
  ```
  $ ./src/qt/bitcoin-qt -prune=-1
  Error: Prune cannot be configured with a negative value.
  bitcoin-qt: qt/bitcoingui.cpp:1369: bool ThreadSafeMessageBox(BitcoinGUI*, const string&, const string&, unsigned int): Assertion `invoked' failed.
  Aborted (core dumped)
  ```

  This PR reverts all commits of bitcoin#17943

  Additional notes: the bug was missed due to dynamic function call `QMetaObject::invokeMethod()` which cannot be checked at compile time. See bitcoin#16348 for more discussion.

  Sorry for introducing a bug.

ACKs for top commit:
  Sjors:
    ACK 70e4706
  laanwj:
    ACK 70e4706

Tree-SHA512: b968a026eaa4f5f39fd36ddc715d8e233f3c6420e6580f11d4ca422a5ff5d1d9d3df9ac11b353c3d4f434d67d6a69e37d2e26b8248d72bedd14ecba0a545a327
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 28, 2021
@fanquake @PastaPastaPasta
Merge bitcoin#17943: qt, refactor: Remove never used default parameter
5dbec51 
1a53b0d refactor: Simplify connection syntax (Hennadii Stepanov)
7d0a8f4 refactor: Remove never used default parameter (Hennadii Stepanov)

Pull request description:

  In `BitcoinGUI::message()` slot the `bool* ret = nullptr` parameter is never used.

  This PR removes it and simplifies connections syntax by replacing lambdas with the `&BitcoinGUI::message` slot.

ACKs for top commit:
  promag:
    Code review ACK 1a53b0d.
  Sjors:
    Tested ACK 1a53b0d
  Empact:
    Code review ACK bitcoin@1a53b0d

Tree-SHA512: e287c3218d31a387338d50da3de79c27e8691829449c3a75a2f75bb1c680bd81eb9de43e4dd3646560a422d4a45c84debfce9783c4376b50aa5cde491f300688
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 28, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#17965: qt: Revert changes of pr17943
a60f648 
70e4706 Revert "refactor: Remove never used default parameter" (Hennadii Stepanov)
219417b Revert "refactor: Simplify connection syntax" (Hennadii Stepanov)

Pull request description:

  The code, the `bool* ret = nullptr` parameter in the `BitcoinGUI::message()` slot, removed in bitcoin#17943 is not dead actually. It is used in `ThreadSafeMessageBox()` function:
  https://github.com/bitcoin/bitcoin/blob/a654626f076a72416a3d354218d7107571d6caaf/src/qt/bitcoingui.cpp#L1363-L1368

  Now in master (a654626):
  ```
  $ ./src/qt/bitcoin-qt -prune=-1
  Error: Prune cannot be configured with a negative value.
  bitcoin-qt: qt/bitcoingui.cpp:1369: bool ThreadSafeMessageBox(BitcoinGUI*, const string&, const string&, unsigned int): Assertion `invoked' failed.
  Aborted (core dumped)
  ```

  This PR reverts all commits of bitcoin#17943

  Additional notes: the bug was missed due to dynamic function call `QMetaObject::invokeMethod()` which cannot be checked at compile time. See bitcoin#16348 for more discussion.

  Sorry for introducing a bug.

ACKs for top commit:
  Sjors:
    ACK 70e4706
  laanwj:
    ACK 70e4706

Tree-SHA512: b968a026eaa4f5f39fd36ddc715d8e233f3c6420e6580f11d4ca422a5ff5d1d9d3df9ac11b353c3d4f434d67d6a69e37d2e26b8248d72bedd14ecba0a545a327
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
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
GUI Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hebasto @promag @DrahtBot @Empact @Sjors @fanquake