Part of #30289. Inspired by #28676 (review).
Since there has been more than one case in the development of #28676 of calling a TxGraph function without correctly setting the bool main_only argument that many of its interface functions have, make these mandatory and explicit, using an enum class Level:
enum class Level {
TOP, //!< Refers to staging if it exists, main otherwise.
MAIN //!< Always refers to the main graph, whether staging is present or not.
};
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33354.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | vasild, instagibbs, glozow |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Possible typos and grammar issues:
<sup>drahtbot_id_5_m</sup>
1172 | - for (int main_only = 0; main_only < 2; ++main_only) { 1173 | - auto& sim = main_only ? sims[0] : sims.back(); 1174 | + // Try to run a full comparison, for both TxGraph::Level::MAIN and TxGraph::Level::TOP in 1175 | + // TxGraph inspector functions that support both. 1176 | + for (int level_select_int = 0; level_select_int < 2; ++level_select_int) { 1177 | + TxGraph::Level level = level_select_int == 0 ? TxGraph::Level::TOP : TxGraph::Level::MAIN;
Can be written as:
for (auto level : { TxGraph::Level::TOP, TxGraph::Level::MAIN }) {
Indeed, done!
ACK b9af83e72c60c1ee71251cf9f3dbae0ffccbe305
Passing enum values with meaningful names seems better and more readable than passing true / false or omitting the argument.
ACK d45f3717d2c65d1a6012a4bc2f47ff75004fd171
concept ACK, having the two values was never confusing, it just took mental load to figure out what the implied argument meant in each context (and lead to bugs too)
ACK d45f3717d2c65d1a6012a4bc2f47ff75004fd171
significantly easier to read usages, thank you
code review ACK d45f3717d2c65d1a6012a4bc2f47ff75004fd171