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

refactor: Use enum type as switch argument in *TableModel #166

Merged
merged 4 commits into from
Mar 7, 2021

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Dec 25, 2020

This PR makes code more maintainable by leveraging -Wswitch compiler warnings.

Only the RecentRequestsTableModel is not refactored, because its enum ColumnIndex contains additional NUMBER_OF_COLUMNS value.

No behavior change.

@hebasto
Copy link
Member Author

hebasto commented Dec 30, 2020

Rebased aeb11d3 -> dae033e (pr166.01 -> pr166.02) due to the conflict with #162.

@hebasto
Copy link
Member Author

hebasto commented Jan 11, 2021

Rebased dae033e -> d4508f1 (pr166.02 -> pr166.03) due to the conflict with #161.

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Concept ACK.

It's not clear to me if data() implementation should check for out of bounds. I think Qt views and proxy model behave correctly by querying data inside columnCount() and rowCount().

As follow up:

  • move AddressTableModel::columns member to priv instance
  • drop section < columns.size() check in AddressTableModel::headerData

@hebasto
Copy link
Member Author

hebasto commented Feb 21, 2021

@promag

It's not clear to me if data() implementation should check for out of bounds. I think Qt views and proxy model behave correctly by querying data inside columnCount() and rowCount().

At least, these changes do not affect the current behavior, right?

@hebasto
Copy link
Member Author

hebasto commented Feb 21, 2021

@jonasschnelli Maybe tag this pr with 22.0 milestone?

@hebasto
Copy link
Member Author

hebasto commented Feb 22, 2021

Rebased d4508f1 -> 1d5d832 (pr166.03 -> pr166.04) due to the conflict with #179.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

ACK 1d5d832, tested on macOS 11.1 Qt 5.15.2

This refactor brings a safer switch statement design to the address, ban, peer, and transaction table models. It also brings us in accordance with the switch design recommendation included in developer-notes.md.

The ability to leverage the -Wswitch compiler warning, as mentioned in the OP, is another benefit of this refactor. An example of the benefit this brings can be shown by considering a scenario where a new Address type is introduced, and the author forgets to update the code to consider the new type. In such a scenario, the compiler will warn about this, thus preventing errors.

Copy link

@leonardojobim leonardojobim left a comment

Choose a reason for hiding this comment

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

ACK 1d5d832, tested on Ubuntu 20.04 Qt 5.12.8

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Code review ACK 1d5d832.

@hebasto
Copy link
Member Author

hebasto commented Feb 26, 2021

@leonardojobim

ACK ab8a747, tested on Ubuntu 20.04 Qt 5.12.8

Thank you for your review!

Do you mind mentioning the top pr commit with your ACK, i.e., 1d5d832, not ab8a747?

@leonardojobim
Copy link

No problem. Changed it to the top PR commit.

@maflcko maflcko merged commit 8c21562 into bitcoin-core:master Mar 7, 2021
@hebasto hebasto deleted the 201225-header branch March 7, 2021 12:12
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 7, 2021
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 28, 2022
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
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.

6 participants