Skip to content

Conversation

@knst
Copy link
Collaborator

@knst knst commented Feb 14, 2023

Issue being fixed or feature implemented

expressions like nType == MnType::HighPerformance.index look pretty confusing in current implementation of 4k HPMN.

Changing uint8_t index to enum class MnType : uint8_t give pros:

  • switch inside GetMnType() and any similar code will show a compiler warning if any type is missing.
  • instead "MnType::HighPerformance.index" you can write MnType::HighPerformance
  • you can remove confusing .index from MnType

But also Cons:

  • instead log("%d", nType) you need to write log("%d", static_cast<int>(nType));

What was done?

Introduced new enum class MnType and rewritten generating Regular/HighPerformance objects with params (description, collateral amount, etc).

Also were added attributes [[no_discard]] for related code.

How Has This Been Tested?

Run unit/functional tests

Breaking Changes

No breaking changes.

Checklist:

  • I have performed a self-review of my own code
  • I have assigned this pull request to a milestone

@knst knst added this to the 19 milestone Feb 14, 2023
@knst knst requested a review from ogabrielides February 14, 2023 20:16
@knst knst marked this pull request as ready for review February 14, 2023 20:17
ogabrielides
ogabrielides previously approved these changes Feb 15, 2023
Copy link

@ogabrielides ogabrielides left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

LGTM, one nit

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

LGTM, utACK

Copy link

@ogabrielides ogabrielides left a comment

Choose a reason for hiding this comment

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

LGTM, utACK

@ogabrielides ogabrielides merged commit 2083380 into dashpay:develop Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants