ronantakizawa/codereview-bench huggingface dataset:
Repository: bitcoin/bitcoin
File: src/util/expected.h (C/C++)

Code before:

#include <attributes.h>
#include <util/check.h>

#include <cassert>
#include <exception>
#include <type_traits>
#include <utility>
#include <variant>

namespace util {

/// The util::Unexpected class represents an unexpected value stored in
/// util::Expected.
template <class E>
class Unexpected
{
public:
    constexpr explicit Unexpected(E e) : m_err(std::move(e)) {}

    constexpr const E& error() const& LIFETIMEBOUND { return m_err; }
    constexpr E& error() & LIFETIMEBOUND { return m_err; }
    constexpr E&& error() && LIFETIMEBOUND { return std::move(m_err); }

private:
    E m_err;
};

struct BadExpectedAccess : std::exception {
    const char* what() const noexcept override { return "Bad util::Expected access"; }
};

/// The util::Expected class provides a standard way for low-level functions to
/// return either error values or result values.
///
/// It provides a smaller version of std::expected from C++23. Missing features
/// can be added, if needed.
template <class T, class E>
class Expected
{
private:
    using ValueType = std::conditional_t<std::is_same_v<T, void>, std::monostate, T>;
    std::variant<ValueType, E> m_data;

public:
    constexpr Expected() : m_data{std::in_place_index_t<0>{}, ValueType{}} {}
    constexpr Expected(ValueType v) : m_data{std::in_place_index_t<0>{}, std::move(v)} {}
    template <class Err>
    constexpr Expected(Unexpected<Err> u) : m_data{std::in_place_index_t<1>{}, std::move(u).error()}
    {
    }

Review comment (suggestion):
In commit "util: Add Unexpected::error()" (faa0d23ee75aac289c3766ce6ee27948b0d982d9)

Calling this `m_error` would seem more consistent.

Code after:

#include <attributes.h>
#include <util/check.h>

#include <cassert>
#include <exception>
#include <utility>
#include <variant>

namespace util {

/// The util::Unexpected class represents an unexpected value stored in
/// util::Expected.
template <class E>
class Unexpected
{
public:
    constexpr explicit Unexpected(E e) : m_error(std::move(e)) {}

    constexpr const E& error() const& noexcept LIFETIMEBOUND { return m_error; }
    constexpr E& error() & noexcept LIFETIMEBOUND { return m_error; }
    constexpr E&& error() && noexcept LIFETIMEBOUND { return std::move(m_error); }

private:
    E m_error;
};

struct BadExpectedAccess : std::exception {
    const char* what() const noexcept override { return "Bad util::Expected access"; }
};

/// The util::Expected class provides a standard way for low-level functions to
/// return either error values or result values.
///
/// It provides a smaller version of std::expected from C++23. Missing features
/// can be added, if needed.
template <class T, class E>
class Expected
{
private:
    std::variant<T, E> m_data;

public:
    constexpr Expected() : m_data{std::in_place_index<0>, T{}} {}
    constexpr Expected(T v) : m_data{std::in_place_index<0>, std::move(v)} {}
    template <class Err>
    constexpr Expected(Unexpected<Err> u) : m_data{std::in_place_index<1>, std::move(u).error()}
    {
    }

    constexpr bool has_value() const noexcept { return m_data.index() == 0; }
Repository: bitcoin/bitcoin
File: src/util/expected.h (C/C++)

Code before:
// file COPYING or https://opensource.org/license/mit.

#ifndef BITCOIN_UTIL_EXPECTED_H
#define BITCOIN_UTIL_EXPECTED_H

#include <attributes.h>
#include <util/check.h>

#include <cassert>
#include <exception>
#include <utility>
#include <variant>

namespace util {

/// The util::Unexpected class represents an unexpected value stored in
/// util::Expected.
template <class E>
class Unexpected
{
public:
    constexpr explicit Unexpected(E e) : m_err(std::move(e)) {}

    constexpr const E& error() const& LIFETIMEBOUND { return m_err; }
    constexpr E& error() & LIFETIMEBOUND { return m_err; }
    constexpr E&& error() && LIFETIMEBOUND { return std::move(m_err); }

private:
    E m_err;
};

struct BadExpectedAccess : std::exception {
    const char* what() const noexcept override { return "Bad util::Expected access"; }
};

/// The util::Expected class provides a standard way for low-level functions to
/// return either error values or result values.
///
/// It provides a smaller version of std::expected from C++23. Missing features
/// can be added, if needed.
template <class T, class E>
class Expected
{
private:
    std::variant<T, E> m_data;

public:
    constexpr Expected() : m_data{std::in_place_index<0>, T{}} {}
    constexpr Expected(T v) : m_data{std::in_place_index<0>, std::move(v)} {}
    template <class Err>
    constexpr Expected(Unexpected<Err> u) : m_data{std::in_place_index<1>, std::move(u).error()}

Review comment (refactor):
nit: Should be `noexcept` according to https://en.cppreference.com/w/cpp/utility/expected/unexpected.html#error

Code after:
// file COPYING or https://opensource.org/license/mit.

#ifndef BITCOIN_UTIL_EXPECTED_H
#define BITCOIN_UTIL_EXPECTED_H

#include <attributes.h>
#include <util/check.h>

#include <cassert>
#include <exception>
#include <utility>
#include <variant>

namespace util {

/// The util::Unexpected class represents an unexpected value stored in
/// util::Expected.
template <class E>
class Unexpected
{
public:
    constexpr explicit Unexpected(E e) : m_error(std::move(e)) {}

    constexpr const E& error() const& noexcept LIFETIMEBOUND { return m_error; }
    constexpr E& error() & noexcept LIFETIMEBOUND { return m_error; }
    constexpr E&& error() && noexcept LIFETIMEBOUND { return std::move(m_error); }

private:
    E m_error;
};

struct BadExpectedAccess : std::exception {
    const char* what() const noexcept override { return "Bad util::Expected access"; }
};

/// The util::Expected class provides a standard way for low-level functions to
/// return either error values or result values.
///
/// It provides a smaller version of std::expected from C++23. Missing features
/// can be added, if needed.
template <class T, class E>
class Expected
{
private:
    std::variant<T, E> m_data;

public:
    constexpr Expected() : m_data{std::in_place_index<0>, T{}} {}
    constexpr Expected(T v) : m_data{std::in_place_index<0>, std::move(v)} {}
    template <class Err>
    constexpr Expected(Unexpected<Err> u) : m_data{std::in_place_index<1>, std::move(u).error()}
Repository: bitcoin/bitcoin
File: src/kernel/chain.cpp (C++)

Code before:
// Copyright (c) 2022 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <chain.h>
#include <kernel/chain.h>
#include <kernel/types.h>
#include <kernel/cs_main.h>
#include <sync.h>
#include <uint256.h>

class CBlock;

using kernel::ChainstateRole;

namespace kernel {
interfaces::BlockInfo MakeBlockInfo(const CBlockIndex* index, const CBlock* data)
{
    interfaces::BlockInfo info{index ? *index->phashBlock : uint256::ZERO};
    if (index) {
        info.prev_hash = index->pprev ? index->pprev->phashBlock : nullptr;
        info.height = index->nHeight;
        info.chain_time_max = index->GetBlockTimeMax();
        LOCK(::cs_main);
        info.file_number = index->nFile;
        info.data_pos = index->nDataPos;
    }
    info.data = data;
    return info;
}

Review comment (suggestion):
```diff
--- a/src/kernel/chain.cpp
+++ b/src/kernel/chain.cpp
@@ -2,8 +2,9 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <chain.h>
 #include <kernel/chain.h>
+
+#include <chain.h>
 #include <kernel/types.h>
 #include <kernel/cs_main.h>
 #include <sync.h>
```

Code after:
// Copyright (c) 2022-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <kernel/chain.h>

#include <chain.h>
#include <kernel/cs_main.h>
#include <kernel/types.h>
#include <sync.h>
#include <uint256.h>

class CBlock;

using kernel::ChainstateRole;

namespace kernel {
interfaces::BlockInfo MakeBlockInfo(const CBlockIndex* index, const CBlock* data)
{
    interfaces::BlockInfo info{index ? *index->phashBlock : uint256::ZERO};
    if (index) {
        info.prev_hash = index->pprev ? index->pprev->phashBlock : nullptr;
        info.height = index->nHeight;
        info.chain_time_max = index->GetBlockTimeMax();
        LOCK(::cs_main);
        info.file_number = index->nFile;
        info.data_pos = index->nDataPos;
    }
    info.data = data;
    return info;
Repository: bitcoin/bitcoin
File: src/core_read.cpp (C++)

Code before:
// Copyright (c) 2009-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <core_io.h>

#include <primitives/block.h>
#include <primitives/transaction.h>
#include <script/script.h>
#include <script/sign.h>
#include <serialize.h>
#include <streams.h>
#include <util/result.h>
#include <util/strencodings.h>

#include <algorithm>
#include <string>

using util::SplitString;

namespace {
class OpCodeParser
{
private:
    std::map<std::string, opcodetype> mapOpNames;

public:
    OpCodeParser()
    {
        for (unsigned int op = 0; op <= MAX_OPCODE; ++op) {
            // Allow OP_RESERVED to get into mapOpNames
            if (op < OP_NOP && op != OP_RESERVED) {

Review comment (bug):
it would be good to reduce and report this bug upstream. Otherwise, wide-spread use of iwyu in this code-base seems risky.

Code after:
// Copyright (c) 2009-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <core_io.h>

#include <primitives/block.h> // IWYU pragma: keep
#include <primitives/transaction.h>
#include <script/interpreter.h>
#include <script/script.h>
#include <serialize.h>
#include <streams.h>
#include <util/result.h>
#include <util/strencodings.h>
#include <util/string.h>
#include <util/translation.h>

#include <algorithm>
#include <compare>
#include <cstdint>
#include <exception>
#include <map>
#include <optional>
#include <span>
#include <stdexcept>
#include <utility>
#include <vector>

using util::SplitString;

namespace {
class OpCodeParser
Repository: bitcoin/bitcoin
File: src/bench/bench.h (C/C++)

Code before:
#include <vector>

/*
 * Usage:

static void NameOfYourBenchmarkFunction(benchmark::Bench& bench)
{
    ...do any setup needed...

    bench.run([&] {
         ...do stuff you want to time; refer to src/bench/nanobench.h
            for more information and the options that can be passed here...
    });

    ...do any cleanup needed...
}

BENCHMARK(NameOfYourBenchmarkFunction);

 */

namespace benchmark {

using ankerl::nanobench::Bench;

typedef std::function<void(Bench&)> BenchFunction;

struct Args {
    bool is_list_only;
    bool sanity_check;
    std::chrono::milliseconds min_time;
    std::vector<double> asymptote;
    fs::path output_csv;
    fs::path output_json;
    std::string regex_filter;
    std::vector<std::string> setup_args;
};

class BenchRunner
{
    // maps from "name" -> function
    using BenchmarkMap = std::map<std::string, BenchFunction>;
    static BenchmarkMap& benchmarks();

public:
    BenchRunner(std::string name, BenchFunction func);

    static void RunAll(const Args& args);
};
} // namespace benchmark


Review comment (suggestion):
nit: we might as well update this, [too](https://github.com/bitcoin/bitcoin/pull/34210/changes#r2683629443):

```suggestion
using BenchFunction = std::function<void(Bench&)>;
```

Code after:
/*
 * Usage:

static void NameOfYourBenchmarkFunction(benchmark::Bench& bench)
{
    ...do any setup needed...

    bench.run([&] {
         ...do stuff you want to time; refer to src/bench/nanobench.h
            for more information and the options that can be passed here...
    });

    ...do any cleanup needed...
}

BENCHMARK(NameOfYourBenchmarkFunction);

 */

namespace benchmark {

using ankerl::nanobench::Bench;

using BenchFunction = std::function<void(Bench&)>;

struct Args {
    bool is_list_only;
    bool sanity_check;
    std::chrono::milliseconds min_time;
    std::vector<double> asymptote;
    fs::path output_csv;
    fs::path output_json;
    std::string regex_filter;
    std::vector<std::string> setup_args;
};

class BenchRunner
{
    // maps from "name" -> function
    using BenchmarkMap = std::map<std::string, BenchFunction>;
    static BenchmarkMap& benchmarks();

public:
    BenchRunner(std::string name, BenchFunction func);

    static void RunAll(const Args& args);
};
} // namespace benchmark

// BENCHMARK(foo); expands to:  benchmark::BenchRunner bench_runner_foo{"foo", foo};
#define BENCHMARK(n) \
Repository: bitcoin/bitcoin
File: src/wallet/dump.cpp (C++)

Code before:
            if (checksum.IsNull()) {
                error = _("Error: Missing checksum");
                ret = false;
            } else if (checksum != comp_checksum) {
                error = strprintf(_("Error: Dumpfile checksum does not match. Computed %s, expected %s"), HexStr(comp_checksum), HexStr(checksum));
                ret = false;
            }
        }

        if (ret) {
            batch->TxnCommit();
        } else {
            batch->TxnAbort();
        }

        batch.reset();

        dump_file.close();
    }
    wallet.reset(); // The pointer deleter will close the wallet for us.

    // Remove the wallet dir if we have a failure
    if (!ret) {
        fs::remove_all(wallet_path);
    }

    return ret;
}
} // namespace wallet


Review comment (bug):
```diff
- // Remove the wallet dir if we have a failure
+ // If we have a failure, remove the wallet files and optionally the wallet dir if a named wallet
``` 

Code after:
            if (checksum.IsNull()) {
                error = _("Error: Missing checksum");
                ret = false;
            } else if (checksum != comp_checksum) {
                error = strprintf(_("Error: Dumpfile checksum does not match. Computed %s, expected %s"), HexStr(comp_checksum), HexStr(checksum));
                ret = false;
            }
        }

        if (ret) {
            batch->TxnCommit();
        } else {
            batch->TxnAbort();
        }

        batch.reset();

        dump_file.close();
    }
    // On failure, gather the paths to remove
    std::vector<fs::path> paths_to_remove = wallet->GetDatabase().Files();
    if (!name.empty()) paths_to_remove.push_back(wallet_path);

    wallet.reset(); // The pointer deleter will close the wallet for us.

    // Remove the wallet dir if we have a failure
    if (!ret) {
        for (const auto& p : paths_to_remove) {
            fs::remove(p);
        }
    }

    return ret;
}
} // namespace wallet
Repository: bitcoin/bitcoin
File: src/wallet/dump.cpp (C++)

Code before:
            } else if (checksum != comp_checksum) {
                error = strprintf(_("Error: Dumpfile checksum does not match. Computed %s, expected %s"), HexStr(comp_checksum), HexStr(checksum));
                ret = false;
            }
        }

        if (ret) {
            batch->TxnCommit();
        } else {
            batch->TxnAbort();
        }

        batch.reset();

        dump_file.close();
    }
    wallet.reset(); // The pointer deleter will close the wallet for us.

    // Remove the wallet dir if we have a failure
    if (!ret) {
        fs::remove_all(wallet_path);
    }

    return ret;
}
} // namespace wallet


Review comment (bug):
One remaining unhandled case is that `fs::remove()` of the `wallet_path` fails (silently, as currently implemented) as the directory contains another file.

I can't think how that would be problematic, nor do I think it should be a blocker for this fix.

Code after:
            } else if (checksum != comp_checksum) {
                error = strprintf(_("Error: Dumpfile checksum does not match. Computed %s, expected %s"), HexStr(comp_checksum), HexStr(checksum));
                ret = false;
            }
        }

        if (ret) {
            batch->TxnCommit();
        } else {
            batch->TxnAbort();
        }

        batch.reset();

        dump_file.close();
    }
    // On failure, gather the paths to remove
    std::vector<fs::path> paths_to_remove = wallet->GetDatabase().Files();
    if (!name.empty()) paths_to_remove.push_back(wallet_path);

    wallet.reset(); // The pointer deleter will close the wallet for us.

    // Remove the wallet dir if we have a failure
    if (!ret) {
        for (const auto& p : paths_to_remove) {
            fs::remove(p);
        }
    }

    return ret;
}
} // namespace wallet
Repository: bitcoin/bitcoin
File: src/netbase.cpp (C++)

Code before:
    case 6667:  // Standard IRC
    case 6668:  // Alternate IRC
    case 6669:  // Alternate IRC
    case 6697:  // IRC + TLS
    case 10080: // Amanda
    case 27017: // MongoDB
        return true;
    }
    return false;
}

CService MaybeFlipIPv6toCJDNS(const CService& service)
{
    CService ret{service};
    if (ret.IsIPv6() && ret.HasCJDNSPrefix() && g_reachable_nets.Contains(NET_CJDNS)) {
        ret.m_net = NET_CJDNS;
    }
    return ret;
}

CService GetBindAddress(const Sock& sock)
{
    CService addr_bind;
    struct sockaddr_storage sockaddr_bind;
    socklen_t sockaddr_bind_len = sizeof(sockaddr_bind);
    if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {
        addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind, sockaddr_bind_len);
    } else {
        LogWarning("getsockname failed\n");
    }
    return addr_bind;
}


Review comment (suggestion):
`GetSockName()` currently forwards the raw `getsockname()` return value (0 on success), which forces callers to use an inverted check (`!GetSockName(...)`).

Can we normalize it so it returns true on success?
```
bool Sock::GetSockName(sockaddr* name, socklen_t* name_len) const
{
    return getsockname(m_socket, name, name_len) == 0;
}
```

It would make the code slightly more readable and maintainable.

Code after:
    case 6667:  // Standard IRC
    case 6668:  // Alternate IRC
    case 6669:  // Alternate IRC
    case 6697:  // IRC + TLS
    case 10080: // Amanda
    case 27017: // MongoDB
        return true;
    }
    return false;
}

CService MaybeFlipIPv6toCJDNS(const CService& service)
{
    CService ret{service};
    if (ret.IsIPv6() && ret.HasCJDNSPrefix() && g_reachable_nets.Contains(NET_CJDNS)) {
        ret.m_net = NET_CJDNS;
    }
    return ret;
}

CService GetBindAddress(const Sock& sock)
{
    CService addr_bind;
    sockaddr_storage storage;
    socklen_t len = sizeof(storage);

    auto sa = reinterpret_cast<sockaddr*>(&storage);

    if (sock.GetSockName(sa, &len) == 0) {
        addr_bind.SetSockAddr(sa, len);
    } else {
        LogWarning("getsockname failed\n");
    }
    return addr_bind;
}
Repository: bitcoin/bitcoin
File: src/netbase.cpp (C++)

Code before:
    case 6668:  // Alternate IRC
    case 6669:  // Alternate IRC
    case 6697:  // IRC + TLS
    case 10080: // Amanda
    case 27017: // MongoDB
        return true;
    }
    return false;
}

CService MaybeFlipIPv6toCJDNS(const CService& service)
{
    CService ret{service};
    if (ret.IsIPv6() && ret.HasCJDNSPrefix() && g_reachable_nets.Contains(NET_CJDNS)) {
        ret.m_net = NET_CJDNS;
    }
    return ret;
}

CService GetBindAddress(const Sock& sock)
{
    CService addr_bind;
    sockaddr_storage storage;
    socklen_t len = sizeof(storage);

    auto sa = static_cast<sockaddr*>(static_cast<void*>(&storage));

    // POSIX convention: 0 is success
    if (sock.GetSockName(sa, &len) == 0) {
        addr_bind.SetSockAddr(sa, len);
    } else {
        LogWarning("getsockname failed\n");
    }
    return addr_bind;
}


Review comment (suggestion):
The inner cast is unnecessary?

```suggestion
    auto sa = static_cast<sockaddr*>(&storage);
```

Code after:
    case 6668:  // Alternate IRC
    case 6669:  // Alternate IRC
    case 6697:  // IRC + TLS
    case 10080: // Amanda
    case 27017: // MongoDB
        return true;
    }
    return false;
}

CService MaybeFlipIPv6toCJDNS(const CService& service)
{
    CService ret{service};
    if (ret.IsIPv6() && ret.HasCJDNSPrefix() && g_reachable_nets.Contains(NET_CJDNS)) {
        ret.m_net = NET_CJDNS;
    }
    return ret;
}

CService GetBindAddress(const Sock& sock)
{
    CService addr_bind;
    sockaddr_storage storage;
    socklen_t len = sizeof(storage);

    auto sa = reinterpret_cast<sockaddr*>(&storage);

    if (sock.GetSockName(sa, &len) == 0) {
        addr_bind.SetSockAddr(sa, len);
    } else {
        LogWarning("getsockname failed\n");
    }
    return addr_bind;
}
Repository: bitcoin/bitcoin
File: src/netbase.cpp (C++)

Code before:
    case 6697:  // IRC + TLS
    case 10080: // Amanda
    case 27017: // MongoDB
        return true;
    }
    return false;
}

CService MaybeFlipIPv6toCJDNS(const CService& service)
{
    CService ret{service};
    if (ret.IsIPv6() && ret.HasCJDNSPrefix() && g_reachable_nets.Contains(NET_CJDNS)) {
        ret.m_net = NET_CJDNS;
    }
    return ret;
}

CService GetBindAddress(const Sock& sock)
{
    CService addr_bind;
    sockaddr_storage storage;
    socklen_t len = sizeof(storage);

    auto sa = reinterpret_cast<sockaddr*>(&storage);

    // POSIX convention: 0 is success
    if (sock.GetSockName(sa, &len) == 0) {
        addr_bind.SetSockAddr(sa, len);
    } else {
        LogWarning("getsockname failed\n");
    }
    return addr_bind;
}


Review comment (suggestion):
nit in e93778dd82795060288ebda3897c8695386101e8: I think this comment can be removed. It should be self-explanatory via the `_ == 0` below. There are other places that also call `GetSockName` without explaining the return type and just comparing it to zero.

Code after:
    case 6697:  // IRC + TLS
    case 10080: // Amanda
    case 27017: // MongoDB
        return true;
    }
    return false;
}

CService MaybeFlipIPv6toCJDNS(const CService& service)
{
    CService ret{service};
    if (ret.IsIPv6() && ret.HasCJDNSPrefix() && g_reachable_nets.Contains(NET_CJDNS)) {
        ret.m_net = NET_CJDNS;
    }
    return ret;
}

CService GetBindAddress(const Sock& sock)
{
    CService addr_bind;
    sockaddr_storage storage;
    socklen_t len = sizeof(storage);

    auto sa = reinterpret_cast<sockaddr*>(&storage);

    if (sock.GetSockName(sa, &len) == 0) {
        addr_bind.SetSockAddr(sa, len);
    } else {
        LogWarning("getsockname failed\n");
    }
    return addr_bind;
}