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

pull hebasto wants to merge 4 commits into bitcoin-core:master from hebasto:201225-header changing 4 files +65 −65

hebasto commented at 8:59 PM on December 25, 2020: member

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.
DrahtBot added the label Needs rebase on Dec 28, 2020
hebasto force-pushed on Dec 30, 2020
hebasto commented at 6:42 PM on December 30, 2020: member

Rebased aeb11d3704faa7c2cc71f4e23462e5dcd050c6df -> dae033e3fd5c910e961f5f5553004710fb27dd43 (pr166.01 -> pr166.02) due to the conflict with #162.
DrahtBot removed the label Needs rebase on Dec 30, 2020
DrahtBot added the label Needs rebase on Jan 11, 2021
hebasto force-pushed on Jan 11, 2021
hebasto commented at 10:09 AM on January 11, 2021: member

Rebased dae033e3fd5c910e961f5f5553004710fb27dd43 -> d4508f1e82b2bbf591af2cad25933d5050c404bb (pr166.02 -> pr166.03) due to the conflict with #161.
DrahtBot removed the label Needs rebase on Jan 11, 2021
promag commented at 12:40 PM on January 11, 2021: contributor
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 commented at 7:57 PM on February 21, 2021: member

@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 commented at 7:59 PM on February 21, 2021: member

@jonasschnelli Maybe tag this pr with 22.0 milestone?
DrahtBot added the label Needs rebase on Feb 22, 2021
qt, refactor: Use enum type as switch argument in AddressTableModel ab8a747d1c
qt, refactor: Use enum type as switch argument in BanTableModel a35223f1cd
qt, refactor: Use enum type as switch argument in PeerTableModel 52f122c11f
qt, refactor: Use enum type as switch argument in TransactionTableModel 1d5d832d5c
hebasto force-pushed on Feb 22, 2021
hebasto commented at 7:40 AM on February 22, 2021: member

Rebased d4508f1e82b2bbf591af2cad25933d5050c404bb -> 1d5d832d5c045cbbe3a0f4aa8fc29e52ecadc182 (pr166.03 -> pr166.04) due to the conflict with #179.
DrahtBot removed the label Needs rebase on Feb 22, 2021
jarolrod commented at 8:01 PM on February 22, 2021: member

ACK 1d5d832d5c045cbbe3a0f4aa8fc29e52ecadc182, 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.
leonardojobim commented at 9:46 AM on February 26, 2021: none

ACK https://github.com/bitcoin-core/gui/pull/166/commits/1d5d832d5c045cbbe3a0f4aa8fc29e52ecadc182, tested on Ubuntu 20.04 Qt 5.12.8
promag commented at 4:16 PM on February 26, 2021: contributor

Code review ACK 1d5d832d5c045cbbe3a0f4aa8fc29e52ecadc182.
hebasto commented at 4:26 PM on February 26, 2021: member

@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., 1d5d832d5c045cbbe3a0f4aa8fc29e52ecadc182, not ab8a747d1ced9f20ca32f9898418be70670da71a?
leonardojobim commented at 10:34 PM on February 26, 2021: none

No problem. Changed it to the top PR commit.
hebasto added the label Refactoring on Mar 6, 2021
MarcoFalke merged this on Mar 7, 2021
MarcoFalke closed this on Mar 7, 2021
hebasto deleted the branch on Mar 7, 2021
sidhujag referenced this in commit 81130fd6a9 on Mar 7, 2021
gwillen referenced this in commit 2a4f512a08 on Jun 28, 2022
bitcoin-core locked this on Aug 16, 2022

Contributors

Labels