refactor: convert maybe_error into generic `Result` template class #5109

PastaPastaPasta · 2022-12-15T05:58:08Z

Issue being fixed or feature implemented

Removed non-generic maybe_error and replaced with a generic Result class paired with Ok and Err structs

What was done?

This is a simple implementation of the Result type in C++. The Result type is a type that can either be an Ok value, which represents a successful operation and contains a value, or an Err value, which represents a failed operation and contains an error value.

The Result class is a template class that takes two types: T, which is the type of the value that an Ok result will contain, and E, which is the type of the value that an Err result will contain.

The Ok and Err structs are also templates, and are used to construct Ok and Err values. The Ok struct has a single member val_ of type T, and the Err struct has a single member val_ of type E.

The Result class has a constructor that takes either an Ok value or an Err value, and stores the value internally. It also has several member functions that can be used to access the value of the Result and check whether it is an Ok or Err value.

The is_ok() and is_err() member functions can be used to check whether the Result is an Ok or Err value, respectively. The operator bool() function allows the Result to be used in a boolean context, such as an if statement. The unwrap() and unwrap_err() member functions can be used to get the value contained in the Result, but will assert if the Result is not of the expected type.

How Has This Been Tested?

building, unit tests

Breaking Changes

Should be none

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation

For repository code-owners and collaborators only

I have assigned this pull request to a milestone

knst

My concern, that class Result always allocate memory for both type Ok and E.

    Ok<T> val_;
    E err_;

So far as I can see all Ok<> and E<> requires default construct that will be called whatever Result succeed case or not.
Also it always consume memory for both Ok and E even if it is only one needed.

That's fine if it is an implementation for small and rarely used component.
But if the Result is supposed to be general util component it will spread someday all over code base and will effect both performance and memory consumption and it is far from production ready.

I think wrapper over std::variant will work better.

PastaPastaPasta · 2022-12-15T15:18:33Z

My concern, that class Result always allocate memory for both type Ok and E.
    Ok<T> val_;

    E err_;
So far as I can see all Ok<> and E<> requires default construct that will be called whatever Result succeed case or not.

Also it always consume memory for both Ok and E even if it is only one needed.

That's fine if it is an implementation for small and rarely used component.

But if the Result is supposed to be general util component it will spread someday all over code base and will effect both performance and memory consumption and it is far from production ready.

I think wrapper over std::variant will work better.

I am worried about this. I think that we could add some static asserts which ensure the types are trivially constructible.

PastaPastaPasta · 2022-12-15T16:40:47Z

Variant seems to work fine, how does that look?

knst

Please, check this branch: develop...knst:dash:refac/result

src/evo/providertx.h

github-actions · 2022-12-30T05:47:53Z

This pull request has conflicts, please rebase.

github-actions · 2023-02-03T05:24:05Z

This pull request has conflicts, please rebase.

@PastaPastaPasta

## Issue being fixed or feature implemented This refactoring helps to make code more specific and clear. There's using syntax feature from modern C++ such as 'enum class', structure bindings in loops, declaration variables inside if/switch statements, etc. ## What was done? This PR is based on @PastaPastaPasta 's PR #4472 There excluded changes related to using std::optional. Let's decide firstly about `Result` class: #5109 ## How Has This Been Tested? Run unit/functional tests ## Breaking Changes No breaking changes ## Checklist: - [x] I have performed a self-review of my own code - [x] I have assigned this pull request to a milestone --------- Co-authored-by: Pasta <[email protected]>

knst

utACK

src/util/result.h

…lementation Since last refactoring bitcoin#15921 and bitcoin#17004 the benefits of using maybe_error are less significants because the interface became simpler. In future it can be considered using universal `maybe_error` (Result) as proposed in PR dashpay#5109 instead this particular implementation

UdjinM6

utACK

…lementation Since last refactoring bitcoin#15921 and bitcoin#17004 the benefits of using maybe_error are less significants because the interface became simpler. In future it can be considered using universal `maybe_error` (Result) as proposed in PR dashpay#5109 instead this particular implementation

knst

LGTM

let's wait for cd2901e/5309

…lementation Since last refactoring bitcoin#15921 and bitcoin#17004 the benefits of using maybe_error are less significants because the interface became simpler. In future it can be considered using universal `maybe_error` (Result) as proposed in PR dashpay#5109 instead this particular implementation

github-actions · 2023-04-17T15:45:59Z

This pull request has conflicts, please rebase.

…lementation Since last refactoring bitcoin#15921 and bitcoin#17004 the benefits of using maybe_error are less significants because the interface became simpler. In future it can be considered using universal `maybe_error` (Result) as proposed in PR dashpay#5109 instead this particular implementation

…h bitcoin implementation (#5335) ## Issue being fixed or feature implemented The benefits of using custom struct `maybe_error` is less significant since the interface `TxValidationState` became simpler after refactorings from bitcoin#15921 and bitcoin#17004 The refactoring of `maybe_error` as a class result from PR #5109 is still useful but not for case of TxValidationState. ## What was done? Unified using TxValidationState in dash's related code. ## How Has This Been Tested? Run unit/functional tests ## Breaking Changes No breaking changes, logic are same. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone

github-actions · 2023-04-25T20:17:46Z

This pull request has conflicts, please rebase.

knst · 2023-07-24T16:17:15Z

Let's get this one merged! @PastaPastaPasta
Please, check this commit - knst@2bd3d06

p.s. The usages from validation.cpp and evo/ has been replaced to BlockValidationState and TxValidationState to follow bitcoin's changes.

This is a simple implementation of the Result type in C++. The Result type is a type that can either be an Ok value, which represents a successful operation and contains a value, or an Err value, which represents a failed operation and contains an error value. The Result class is a template class that takes two types: T, which is the type of the value that an Ok result will contain, and E, which is the type of the value that an Err result will contain. The Ok and Err structs are also templates, and are used to construct Ok and Err values. The Ok struct has an optional single member val_ of type T, and the Err struct has a single member val_ of type E. The Result class has a constructor that takes either an Ok value or an Err value, and stores the value internally. It also has several member functions that can be used to access the value of the Result and check whether it is an Ok or Err value. The is_ok() and is_err() member functions can be used to check whether the Result is an Ok or Err value, respectively. The operator bool() function allows the Result to be used in a boolean context, such as an if statement. The unwrap() and err() member functions can be used to get the value contained in the Result, but will assert if the Result is not of the expected type. For parametrizing maybe_error with Ok without value, should be used template Ok<void>, you can return just {} in this case as success. Co-Authored-By: Konstantin Akimov <[email protected]>

PastaPastaPasta · 2023-09-29T14:38:30Z

Reviving this PR, currently with no usage; I would like to point towards an implementation of std::expected (which operated similar to Result) here: https://github.com/TartanLlama/expected

please provide conceptual review, especially on if we want to use a Result type class or tl::expected (https://en.cppreference.com/w/cpp/utility/expected)

knst · 2024-01-13T21:10:12Z

Reviving this PR, currently with no usage; I would like to point towards an implementation of std::expected

std::expected is indeed a better option.

Super-seeded by https://github.com/dashpay/dash/blob/develop/src/util/expected.h from #5782

PastaPastaPasta added this to the 19 milestone Dec 15, 2022

knst reviewed Dec 15, 2022

View reviewed changes

PastaPastaPasta requested a review from knst December 15, 2022 16:40

knst requested changes Dec 19, 2022

View reviewed changes

src/evo/providertx.h Outdated Show resolved Hide resolved

github-actions bot added the needs rebase label Dec 30, 2022

PastaPastaPasta force-pushed the refac/result branch from 3fc0960 to 2120a87 Compare January 24, 2023 16:58

github-actions bot removed the needs rebase label Jan 24, 2023

github-actions bot added the needs rebase label Feb 3, 2023

This was referenced Feb 8, 2023

refactor: using C++11/14/17 features in dash specifict code #5190

Merged

Cxx17 refac #4472

Closed

UdjinM6 modified the milestones: 19, 20 Feb 27, 2023

PastaPastaPasta force-pushed the refac/result branch from 2120a87 to 111a675 Compare March 6, 2023 16:13

github-actions bot removed the needs rebase label Mar 6, 2023

knst previously approved these changes Mar 6, 2023

View reviewed changes

src/util/result.h Outdated Show resolved Hide resolved

PastaPastaPasta requested a review from UdjinM6 March 20, 2023 15:27

PastaPastaPasta dismissed knst’s stale review via 2a9c11b April 15, 2023 14:32

PastaPastaPasta force-pushed the refac/result branch from 111a675 to 2a9c11b Compare April 15, 2023 14:32

UdjinM6 previously approved these changes Apr 15, 2023

View reviewed changes

UdjinM6 force-pushed the refac/result branch from 50c04fb to 6308c2a Compare April 15, 2023 17:29

knst previously approved these changes Apr 15, 2023

View reviewed changes

UdjinM6 dismissed knst’s stale review via 7f6a3fe April 15, 2023 20:29

UdjinM6 force-pushed the refac/result branch from 6308c2a to 7f6a3fe Compare April 15, 2023 20:29

PastaPastaPasta mentioned this pull request Apr 17, 2023

backport: bitcoin#15921, #16507, #16524, #16889, #16911, #17004, #17195, #17624, partial #17212 #5309

Merged

3 tasks

github-actions bot added the needs rebase label Apr 17, 2023

knst mentioned this pull request Apr 17, 2023

refactor: prefer using TxValidationState directly for unification with bitcoin implementation #5335

Merged

5 tasks

PastaPastaPasta force-pushed the refac/result branch from 7f6a3fe to c10833a Compare April 18, 2023 05:10

github-actions bot removed the needs rebase label Apr 18, 2023

github-actions bot added the needs rebase label Apr 25, 2023

PastaPastaPasta force-pushed the refac/result branch from 7803828 to 5c345e1 Compare September 29, 2023 14:37

github-actions bot removed the needs rebase label Sep 29, 2023

knst modified the milestones: 20, 21 Oct 23, 2023

UdjinM6 modified the milestones: 21, 20.1 Nov 14, 2023

knst mentioned this pull request Dec 21, 2023

refactor: remove circular dependencies through net_processing (1/N) #5782

Merged

5 tasks

knst closed this Jan 13, 2024

UdjinM6 removed this from the 20.1 milestone Jan 17, 2024

refactor: convert maybe_error into generic Result template class #5109

refactor: convert maybe_error into generic Result template class #5109

Uh oh!

Conversation

PastaPastaPasta commented Dec 15, 2022

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented Dec 15, 2022

Uh oh!

PastaPastaPasta commented Dec 15, 2022

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Dec 30, 2022

Uh oh!

github-actions bot commented Feb 3, 2023

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 17, 2023

Uh oh!

github-actions bot commented Apr 25, 2023

Uh oh!

knst commented Jul 24, 2023

Uh oh!

PastaPastaPasta commented Sep 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knst commented Jan 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

refactor: convert maybe_error into generic `Result` template class #5109

refactor: convert maybe_error into generic `Result` template class #5109

PastaPastaPasta commented Sep 29, 2023 •

edited

Loading