Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disable util::Result copying and assignment #29906

Merged
merged 2 commits into from Apr 30, 2024
Merged

Disable util::Result copying and assignment #29906

merged 2 commits into from Apr 30, 2024

Commits on Apr 25, 2024

  1. refactor: Drop util::Result operator= 

    `util::Result` objects are aggregates that can hold multiple fields with
different information. Currently Result objects can only hold a success value
of an arbitrary type or a single bilingual_str error message. In followup PR
bitcoin#25722, Result objects may be able to
hold both success and failure values of different types, plus error and warning
messages.

Having a Result::operator= assignment operator that completely erases all
existing Result information before assigning new information is potentially
dangerous in this case. For example, code that looks like it is assigning a
warning value could erase previously-assigned success or failure values.
Conversely, code that looks like it is just assigning a success or failure
value could erase previously assigned error and warning messages.

To prevent potential bugs like this, disable Result::operator= assignment
operator.

It is possible in the future we may want to re-enable operator= in limited
cases (such as when implicit conversions are not used) or add a Replace() or
Reset() method that mimicks default operator= behavior. Followup PR
bitcoin#25722 also adds a Result::Update()
method providing another way to update an existing Result object.

Co-authored-by: stickies-v <stickies-v@protonmail.com>
    @ryanofsky @stickies-v
    ryanofsky and stickies-v committed Apr 25, 2024
    Configuration menu
    Copy the full SHA
    834f65e View commit details
    Browse the repository at this point in the history

  2. refactor: Avoid copying util::Result values 

    Copying util::Result values is less efficient than moving them because they
allocate memory and contain strings. Also this is needed to avoid compile
errors in bitcoin#25722 which adds a
std::unique_ptr member to util::Result which implicity disables copying.
    @ryanofsky
    ryanofsky committed Apr 25, 2024
    Configuration menu
    Copy the full SHA
    6a8b2be View commit details
    Browse the repository at this point in the history