📚 Tutorial: refactor: Use more scoped enums by godexsoft · Pull Request #7086 · XRPLF/rippled · GitHub

godexsoft · 2026-05-06T16:53:35Z

High Level Overview of Change

This PR converts more C enums to modern scoped enums.
Remaining enums with NOLINT are best left alone for now. Comments above their respective NOLINT indicate why.

API Impact

No impact.

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

github-actions · 2026-05-07T13:26:55Z

This PR has conflicts, please resolve them in order for the PR to be reviewed.

codecov · 2026-05-07T13:33:58Z

Codecov Report

❌ Patch coverage is 80.34682% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.1%. Comparing base (779b49c) to head (93fdb56).

Files with missing lines	Patch %	Lines
src/libxrpl/basics/Log.cpp	13.8%	25 Missing ⚠️
src/xrpld/app/ledger/LedgerHistory.cpp	0.0%	11 Missing ⚠️
include/xrpl/resource/detail/Logic.h	45.5%	6 Missing ⚠️
src/libxrpl/json/JsonPropertyStream.cpp	0.0%	5 Missing ⚠️
src/libxrpl/json/json_reader.cpp	89.6%	5 Missing ⚠️
src/xrpld/app/ledger/detail/InboundLedger.cpp	44.4%	5 Missing ⚠️
src/libxrpl/beast/utility/beast_Journal.cpp	33.3%	4 Missing ⚠️
src/xrpld/app/ledger/detail/LedgerToJson.cpp	81.0%	4 Missing ⚠️
src/xrpld/app/main/GRPCServer.cpp	0.0%	4 Missing ⚠️
src/xrpld/app/misc/NetworkOPs.cpp	88.9%	4 Missing ⚠️
... and 19 more

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #7086   +/-   ##
=======================================
  Coverage     82.1%   82.1%           
=======================================
  Files         1010    1010           
  Lines        76070   76064    -6     
  Branches      7381    7374    -7     
=======================================
+ Hits         62457   62472   +15     
+ Misses       13613   13592   -21

Files with missing lines	Coverage Δ
include/xrpl/basics/Log.h	`71.4% <ø> (ø)`
include/xrpl/basics/StringUtilities.h	`100.0% <ø> (ø)`
include/xrpl/beast/utility/Journal.h	`96.5% <100.0%> (ø)`
include/xrpl/json/json_reader.h	`100.0% <ø> (ø)`
include/xrpl/json/json_value.h	`98.5% <ø> (ø)`
include/xrpl/json/json_writer.h	`100.0% <100.0%> (ø)`
include/xrpl/ledger/AmendmentTable.h	`100.0% <ø> (ø)`
include/xrpl/protocol/ApiVersion.h	`100.0% <100.0%> (ø)`
include/xrpl/protocol/SField.h	`100.0% <ø> (ø)`
include/xrpl/protocol/STAmount.h	`93.7% <ø> (ø)`
... and 142 more

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…ed-enums

…into refactor/scoped-enums

github-actions · 2026-05-07T14:00:06Z

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

bthomee

I went through all lines to the best of my ability, and only a few things stood out that are mostly unrelated to this PR.

For good measure I used Claude to also perform a scan and it reported that the changed enums etc. were safe and resulted in identical behavior. It also noticed the changes to Beast severities as a breaking API change, but in our case this is fine to ignore.

bthomee · 2026-05-09T14:14:07Z

-                                  // object. Intended for STNumber.
-        SMdDefault = SMdChangeOrig | SMdChangeNew | SMdDeleteFinal | SMdCreate
-    };
+    static constexpr auto kSMD_NEVER = 0x00;


Is having these as constexpr better than as enum class members?

The intention of anonymous enum as used here is exactly to make constants. At the time of writing this was a good way but i believe in modern c++ there are better ways, like static constexpr auto for example :)

bthomee · 2026-05-09T14:17:48Z

+        DisableApiPriorV2 = 0b0000'0010,
+
+        // IMPORTANT `All` must be union of all of the above; see also operator~
+        All = 0b0000'0011


FWIW setting this to All = IncludeDate | DisableApiPriorV2 would be more elegant, and also consistent with how a similar value was set in SField.h.

It's probably outside the scope of this PR though, but at the same time not something anyone would create a dedicated PR for...

I can try to apply that here

bthomee · 2026-05-09T14:25:14Z

-        // MaxPacketSize = 484
-        MaxPacketSize = 1472
-    };
+    // MaxPacketSize = 484


Let's delete this one.

Deleted. i only left it before as it already was the "old value" that was left in a comment for some reason

mathbunnyru · 2026-05-11T09:42:54Z

-        TokenMemberSeparator,
-        TokenComment,
-        TokenError
+    enum class TokenType {


Could you make all enums where it's possible to be uint8_t?

Sounds like an unrelated and slightly dangerous change

mathbunnyru · 2026-05-11T09:59:50Z

            break;

-        case UintValue:
+        case ValueType::Uint:


Suggested change

case ValueType::Uint:

case ValueType::UInt:

While it's not too late (uint is weird for me though - I like uint or UInt)

Let's just fix all Uint in the project while we are here :)

mathbunnyru · 2026-05-11T10:01:46Z

+    static constexpr auto kSMD_ALWAYS = 0x10;    // value when node containing it is affected at all
+    static constexpr auto kSMD_BASE_TEN = 0x20;  // value is treated as base 10, overriding behavior


Let's align comments

Can't align them differently unless we want to clang-format: off

mathbunnyru · 2026-05-11T10:03:37Z

-// Bitwise flags, 86 files
+// Bitwise flags, 86 files, used in macros files
 // NOLINTNEXTLINE(cppcoreguidelines-use-enum-class)
 enum Privilege {


Can't we make this an enum class?
If there are other enums left, please, take a look as well

No and it even says why it was not converted. It would break macros files and i don't want to deal with external scripts in this PR so leaving this for future.

mathbunnyru · 2026-05-11T10:05:52Z

    switch (value.type())
    {
-        case json::NullValue: {
+        case json::ValueType::Null: {


We're in json namespace, so we can probably remove json:: prefix here

…ed-enums

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

mathbunnyru

Nice 👍

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

godexsoft added 11 commits May 6, 2026 14:46

Severity as scoped enum

c74b8a5

Remove deprecated LogSeverity

778d97a

Convert many anonymous enums to static constexpr

92226d1

Convert ValueType to scoped enum

3375e4e

Convert TokenType to scoped enum

0a1f4f9

ValueType names

a54ece0

DuplicationPolicy scoped enum

58b6a95

Json Values better names

0185a1a

RPC::Condition scoped enum

78ef625

ConsequencesFactoryType scoped enum

2c66fb0

Comment about macros

f46ddf1

Copilot AI review requested due to automatic review settings May 6, 2026 16:53

Copilot AI reviewed May 6, 2026

godexsoft added 2 commits May 6, 2026 18:51

Fix clang-tidy

d0b51ce

Merge branch 'develop' into refactor/scoped-enums

92ec566

Copilot AI review requested due to automatic review settings May 7, 2026 12:57

Copilot AI reviewed May 7, 2026

github-actions Bot added the PR: has conflicts label May 7, 2026

godexsoft added 2 commits May 7, 2026 14:59

Merge branch 'develop' of github.com:XRPLF/rippled into refactor/scop…

62cb9d0

…ed-enums

Merge branch 'refactor/scoped-enums' of github.com:godexsoft/rippled …

4b5744b

…into refactor/scoped-enums

github-actions Bot removed the PR: has conflicts label May 7, 2026

bthomee approved these changes May 9, 2026

View reviewed changes

mathbunnyru reviewed May 11, 2026

View reviewed changes

Address review comments

06e37b3

Copilot AI review requested due to automatic review settings May 11, 2026 13:38

Merge branch 'develop' of github.com:XRPLF/rippled into refactor/scop…

a893f52

…ed-enums

Copilot AI reviewed May 11, 2026

godexsoft requested review from bthomee and mathbunnyru May 11, 2026 13:39

mathbunnyru approved these changes May 11, 2026

View reviewed changes

godexsoft added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label May 11, 2026

bthomee approved these changes May 11, 2026

View reviewed changes

Fix build

93fdb56

Copilot AI review requested due to automatic review settings May 11, 2026 14:19

Copilot AI reviewed May 11, 2026

bthomee enabled auto-merge May 11, 2026 14:28

bthomee added this pull request to the merge queue May 11, 2026

Merged via the queue into XRPLF:develop with commit cdee9a6 May 11, 2026
3 checks passed

mvadari added this to the 3.2.0 milestone May 20, 2026

beartec-jpg pushed a commit to beartec-jpg/FalconLedger that referenced this pull request Jun 1, 2026

refactor: Use more scoped enums (XRPLF#7086)

89b4b99

		static constexpr auto kSMD_ALWAYS = 0x10; // value when node containing it is affected at all
		static constexpr auto kSMD_BASE_TEN = 0x20; // value is treated as base 10, overriding behavior

Uh oh!

Conversation

godexsoft commented May 6, 2026

High Level Overview of Change

API Impact

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

codecov Bot commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

bthomee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

mathbunnyru left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented May 7, 2026 •

edited

Loading