refactor: Make BitcoinUnits::Unit a scoped enum #556
Conversation
|
Previous invalidated ACKS: @Sjors tACK |
8df40c8 to
3fb5386
Compare
This change improves type safety.
Since BitcoinUnits::Unit became a scoped enum, BitcoinUnits::valid function is no longer needed.
Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>
I split this out from an earlier commit to make it a bit less noisy. Signed-off-by: William Casarin <jb55@jb55.com>
3fb5386 to
0e5dedb
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
| }; | ||
| typedef BitcoinUnits::Unit BitcoinUnit; | ||
|
|
||
| QDataStream& operator<<(QDataStream& out, const BitcoinUnit& unit); | ||
| QDataStream& operator>>(QDataStream& in, BitcoinUnit& unit); |
There was a problem hiding this comment.
| QDataStream& operator>>(QDataStream& in, BitcoinUnit& unit); | |
| QDataStream& operator>>(const QDataStream& in, BitcoinUnit& unit); |
There was a problem hiding this comment.
I think it is ok as is. See https://doc.qt.io/qt-5.15/qdatastream.html#reading-and-writing-other-qt-classes.
There was a problem hiding this comment.
Sure but there's no reason for an in-param to be passed by reference instead of by reference to const, see https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f16-for-in-parameters-pass-cheaply-copied-types-by-value-and-others-by-reference-to-const. No big deal either way but I think it's clearer to show that it's an in-param that won't be mutated.
| if (!settings.contains("DisplayBitcoinUnit")) { | ||
| settings.setValue("DisplayBitcoinUnit", QVariant::fromValue(BitcoinUnit::BTC)); | ||
| } | ||
| QVariant unit = settings.value("DisplayBitcoinUnit"); |
There was a problem hiding this comment.
75832fd this is just memoizing the value, so why not
| QVariant unit = settings.value("DisplayBitcoinUnit"); | |
| const QVariant& unit = settings.value("DisplayBitcoinUnit"); |
|
ACK 0e5dedb, review and debug build of each commit after rebase on current master, lightly tested running the GUI, changing units a few times, and verifying persistence after restarting Happy to re-ACK if update for the comments above. |
|
Concept ACK |
This is a rebased version of #60
Since Qt 5.5 there are means to register an enum type with the meta-object system (such enum still lacks an ability to interact with QSettings::setValue() and QSettings::value() without defined stream operators).
In order to reduce global namespace polluting and to force strong type checking, this PR makes BitcoinUnits::Unit a scoped enum (typedef BitcoinUnits::Unit BitcoinUnit;).
No behavior change.