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

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

#ifndef BITCOIN_WARNINGS_H
#define BITCOIN_WARNINGS_H

#include <optional>
#include <string>

struct bilingual_str;

void SetMiscWarning(const bilingual_str& warning);
void SetfLargeWorkInvalidChainFound(bool flag);
/** Pass std::nullopt to disable the warning */
void SetMedianTimeOffsetWarning(std::optional<bilingual_str> warning);
/** Format a string that describes several potential problems detected by the core.
 * @param[in] verbose bool
 * - if true, get all warnings separated by <hr />
 * - if false, get the most important warning
 * @returns the warning string
 */
bilingual_str GetWarnings(bool verbose);

#endif // BITCOIN_WARNINGS_H


Review comment (bug):
unrelated follow-up idea: It seems wrong to have the warnings module in the `common` library, when it is using globals to keep track of the warnings.

I guess it would be nice to actually move it to the `node` library, and `node` namespace?

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

#ifndef BITCOIN_WARNINGS_H
#define BITCOIN_WARNINGS_H

#include <optional>
#include <string>
#include <vector>

struct bilingual_str;

void SetMiscWarning(const bilingual_str& warning);
void SetfLargeWorkInvalidChainFound(bool flag);
/** Pass std::nullopt to disable the warning */
void SetMedianTimeOffsetWarning(std::optional<bilingual_str> warning);
/** Return potential problems detected by the node. */
std::vector<bilingual_str> GetWarnings();

#endif // BITCOIN_WARNINGS_H
Repository: bitcoin/bitcoin
File: src/util/subprocess.h (C/C++)

Code before:
#include <sstream>
#include <string>
#include <vector>

#ifdef WIN32
  #include <codecvt>
#endif

extern "C" {
#ifdef WIN32
  #include <windows.h>
  #include <io.h>
  #include <cwchar>

  #define close _close
  #define open _open
  #define fileno _fileno
#else
  #include <sys/wait.h>
  #include <unistd.h>
#endif
  #include <csignal>
  #include <fcntl.h>
  #include <sys/types.h>
}

/*!
 * Getting started with reading this source code.
 * The source is mainly divided into four parts:
 * 1. Exception Classes:
 *    These are very basic exception classes derived from
 *    runtime_error exception.
 *    There are two types of exception thrown from subprocess
 *    library: OSError and CalledProcessError
 *
 * 2. Popen Class
 *    This is the main class the users will deal with. It
 *    provides with all the API's to deal with processes.
 *
 * 3. Util namespace
 *    It includes some helper functions to split/join a string,
 *    reading from file descriptors, waiting on a process, fcntl
 *    options on file descriptors etc.
 *
 * 4. Detail namespace
 *    This includes some metaprogramming and helper classes.
 */


namespace subprocess {


Review comment (suggestion):
Just below here... the `#define` for close/open/fileno breaks using those names in other contexts (eg, C++ stdlib includes, potentially boost, whatever file includes this one, etc). At a minimum, we should probably define them as subprocess_(open|close|fileno) and define those to open/close/fileno for non-Windows too.

Code after:
#include <sstream>
#include <string>
#include <vector>

#if (defined _MSC_VER) || (defined __MINGW32__)
  #define __USING_WINDOWS__
#endif

#ifdef __USING_WINDOWS__
  #include <codecvt>
#endif

extern "C" {
#ifdef __USING_WINDOWS__
  #include <windows.h>
  #include <io.h>
  #include <cwchar>
#else
  #include <sys/wait.h>
  #include <unistd.h>
#endif
  #include <csignal>
  #include <fcntl.h>
  #include <sys/types.h>
}

// The Microsoft C++ compiler issues deprecation warnings
// for the standard POSIX function names.
// Its preferred implementations have a leading underscore.
// See: https://learn.microsoft.com/en-us/cpp/c-runtime-library/compatibility.
#if (defined _MSC_VER)
  #define subprocess_close _close
  #define subprocess_fileno _fileno
  #define subprocess_open _open
  #define subprocess_write _write
#else
  #define subprocess_close close
  #define subprocess_fileno fileno
  #define subprocess_open open
  #define subprocess_write write
#endif

/*!
 * Getting started with reading this source code.
 * The source is mainly divided into four parts:
 * 1. Exception Classes:
 *    These are very basic exception classes derived from
 *    runtime_error exception.
 *    There are two types of exception thrown from subprocess
 *    library: OSError and CalledProcessError
 *
Repository: bitcoin/bitcoin
File: src/randomenv.cpp (C++)

Code before:
#include <cstdint>
#include <cstring>
#include <chrono>
#include <climits>
#include <thread>
#include <vector>

#include <sys/types.h> // must go before a number of other headers

#ifdef WIN32
#include <windows.h>
#include <winreg.h>
#else
#include <fcntl.h>
#include <netinet/in.h>
#include <sys/resource.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/time.h>
#include <sys/utsname.h>
#include <unistd.h>
#endif
#if HAVE_DECL_GETIFADDRS && HAVE_DECL_FREEIFADDRS
#include <ifaddrs.h>
#endif
#if HAVE_SYSCTL
#include <sys/sysctl.h>
#if HAVE_VM_VM_PARAM_H
#include <vm/vm_param.h>
#endif
#if HAVE_SYS_RESOURCES_H
#include <sys/resources.h>
#endif
#if HAVE_SYS_VMMETER_H
#include <sys/vmmeter.h>
#endif
#endif
#if defined(HAVE_STRONG_GETAUXVAL)
#include <sys/auxv.h>
#endif

extern char** environ; // NOLINT(readability-redundant-declaration): Necessary on some platforms

namespace {

void RandAddSeedPerfmon(CSHA512& hasher)
{
#ifdef WIN32
    // Seed with the entire set of perfmon data

    // This can take up to 2 seconds, so only do it every 10 minutes.

Review comment (suggestion):
~~Would it be possible to change these to values 0/1 instead of defined/undefined? It seems to me that changing these to `ifdef` makes it less safe because you lose the protection added by the warning in the first place. (say, when you forget to include bitcoin-config)~~

Code after:
#include <cstdint>
#include <cstring>
#include <chrono>
#include <climits>
#include <thread>
#include <vector>

#include <sys/types.h> // must go before a number of other headers

#ifdef WIN32
#include <windows.h>
#include <winreg.h>
#else
#include <fcntl.h>
#include <netinet/in.h>
#include <sys/resource.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/time.h>
#include <sys/utsname.h>
#include <unistd.h>
#endif
#if HAVE_DECL_GETIFADDRS && HAVE_DECL_FREEIFADDRS
#include <ifaddrs.h>
#endif
#ifdef HAVE_SYSCTL
#include <sys/sysctl.h>
#ifdef HAVE_VM_VM_PARAM_H
#include <vm/vm_param.h>
#endif
#ifdef HAVE_SYS_RESOURCES_H
#include <sys/resources.h>
#endif
#ifdef HAVE_SYS_VMMETER_H
#include <sys/vmmeter.h>
#endif
#endif
#if defined(HAVE_STRONG_GETAUXVAL)
#include <sys/auxv.h>
#endif

extern char** environ; // NOLINT(readability-redundant-declaration): Necessary on some platforms

namespace {

void RandAddSeedPerfmon(CSHA512& hasher)
{
#ifdef WIN32
    // Seed with the entire set of perfmon data

    // This can take up to 2 seconds, so only do it every 10 minutes.
Repository: bitcoin/bitcoin
File: src/common/url.cpp (C++)

Code before:
// Copyright (c) 2015-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 <common/url.h>

#include <event2/http.h>

#include <cstdlib>
#include <string>

std::string urlDecode(const std::string &urlEncoded) {
    std::string res;
    if (!urlEncoded.empty()) {
        char *decoded = evhttp_uridecode(urlEncoded.c_str(), false, nullptr);
        if (decoded) {
            res = std::string(decoded);
            free(decoded);
        }
    }
    return res;
}


Review comment (refactor):
We didn't use [plus decoding](https://github.com/libevent/libevent/blob/e0a4574ba2cbcdb64bb2b593e72be7f7f4010746/http.c#L3505) with `evhttp_uridecode`, so this implementation keeps functionality the same. Since `urlDecode` is a generic utility function, I think there should be a docstring that highlights that this implementation doesn't decode `+` to ` ` in case it's used for other purposes in the future.

Code after:
// Copyright (c) 2015-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 <common/url.h>

#include <charconv>
#include <string>
#include <string_view>
#include <system_error>

std::string UrlDecode(std::string_view url_encoded)
{
    std::string res;
    res.reserve(url_encoded.size());

    for (size_t i = 0; i < url_encoded.size(); ++i) {
        char c = url_encoded[i];
        // Special handling for percent which should be followed by two hex digits
        // representing an octet values, see RFC 3986, Section 2.1 Percent-Encoding
        if (c == '%' && i + 2 < url_encoded.size()) {
            unsigned int decoded_value{0};
            auto [p, ec] = std::from_chars(url_encoded.data() + i + 1, url_encoded.data() + i + 3, decoded_value, 16);

            // Only if there is no error and the pointer is set to the end of
            // the string, we can be sure both characters were valid hex
            if (ec == std::errc{} && p == url_encoded.data() + i + 3) {
                res += static_cast<char>(decoded_value);
                // Next two characters are part of the percent encoding
                i += 2;
                continue;
            }
            // In case of invalid percent encoding, add the '%' and continue
        }
        res += c;
    }
Repository: bitcoin/bitcoin
File: src/common/url.cpp (C++)

Code before:
// Copyright (c) 2015-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 <common/url.h>

#include <event2/http.h>

#include <cstdlib>
#include <string>

std::string urlDecode(const std::string &urlEncoded) {
    std::string res;
    if (!urlEncoded.empty()) {
        char *decoded = evhttp_uridecode(urlEncoded.c_str(), false, nullptr);
        if (decoded) {
            res = std::string(decoded);
            free(decoded);
        }
    }
    return res;
}


Review comment (bug):
What's the rationale for continuing here? Should we accept invalid URLs? At first sight, I think I'd prefer having this return a `util::Result` and have the function fail in case of missing hex digits or invalid hex?

Code after:
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <common/url.h>

#include <charconv>
#include <string>
#include <string_view>
#include <system_error>

std::string UrlDecode(std::string_view url_encoded)
{
    std::string res;
    res.reserve(url_encoded.size());

    for (size_t i = 0; i < url_encoded.size(); ++i) {
        char c = url_encoded[i];
        // Special handling for percent which should be followed by two hex digits
        // representing an octet values, see RFC 3986, Section 2.1 Percent-Encoding
        if (c == '%' && i + 2 < url_encoded.size()) {
            unsigned int decoded_value{0};
            auto [p, ec] = std::from_chars(url_encoded.data() + i + 1, url_encoded.data() + i + 3, decoded_value, 16);

            // Only if there is no error and the pointer is set to the end of
            // the string, we can be sure both characters were valid hex
            if (ec == std::errc{} && p == url_encoded.data() + i + 3) {
                res += static_cast<char>(decoded_value);
                // Next two characters are part of the percent encoding
                i += 2;
                continue;
            }
            // In case of invalid percent encoding, add the '%' and continue
        }
        res += c;
    }

    return res;
}
Repository: bitcoin/bitcoin
File: src/common/url.cpp (C++)

Code before:
// Copyright (c) 2015-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 <common/url.h>

#include <event2/http.h>

#include <cstdlib>
#include <string>

std::string urlDecode(const std::string &urlEncoded) {
    std::string res;
    if (!urlEncoded.empty()) {
        char *decoded = evhttp_uridecode(urlEncoded.c_str(), false, nullptr);
        if (decoded) {
            res = std::string(decoded);
            free(decoded);
        }
    }
    return res;
}


Review comment (suggestion):
`ptr` seems unused, we might as well inline it to:
```C++
std::from_chars(&url_encoded[i + 1], &url_encoded[i + 3], decoded_value, 16).ec == std::errc{})
```
(note, I've used `std::errc{}` instead, seems we're using that more often in the codebase)

Code after:
// Copyright (c) 2015-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 <common/url.h>

#include <charconv>
#include <string>
#include <string_view>
#include <system_error>

std::string UrlDecode(std::string_view url_encoded)
{
    std::string res;
    res.reserve(url_encoded.size());

    for (size_t i = 0; i < url_encoded.size(); ++i) {
        char c = url_encoded[i];
        // Special handling for percent which should be followed by two hex digits
        // representing an octet values, see RFC 3986, Section 2.1 Percent-Encoding
        if (c == '%' && i + 2 < url_encoded.size()) {
            unsigned int decoded_value{0};
            auto [p, ec] = std::from_chars(url_encoded.data() + i + 1, url_encoded.data() + i + 3, decoded_value, 16);

            // Only if there is no error and the pointer is set to the end of
            // the string, we can be sure both characters were valid hex
            if (ec == std::errc{} && p == url_encoded.data() + i + 3) {
                res += static_cast<char>(decoded_value);
                // Next two characters are part of the percent encoding
                i += 2;
                continue;
            }
            // In case of invalid percent encoding, add the '%' and continue
        }
        res += c;
    }

    return res;
}
Repository: bitcoin/bitcoin
File: src/common/url.cpp (C++)

Code before:
// Copyright (c) 2015-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 <common/url.h>

#include <event2/http.h>

#include <cstdlib>
#include <string>

std::string urlDecode(const std::string &urlEncoded) {
    std::string res;
    if (!urlEncoded.empty()) {
        char *decoded = evhttp_uridecode(urlEncoded.c_str(), false, nullptr);
        if (decoded) {
            res = std::string(decoded);
            free(decoded);
        }
    }
    return res;
}


Review comment (suggestion):
we could unify these two `res += c` cases by merging the two conditions:
```C++
std::string UrlDecode(const std::string& url_encoded) {
    std::string res;
    res.reserve(url_encoded.size());

    for (size_t i = 0; i < url_encoded.size(); ++i) {
        int decoded_value = 0;
        if (url_encoded[i] == '%' &&
            i + 2 < url_encoded.size() &&
            std::from_chars(&url_encoded[i + 1], &url_encoded[i + 3], decoded_value, 16).ec == std::errc{}) {
            res += static_cast<char>(decoded_value);
            i += 2;
        } else {
            res += url_encoded[i];
        }
    }

    return res;
}
```

Code after:

#include <common/url.h>

#include <charconv>
#include <string>
#include <string_view>
#include <system_error>

std::string UrlDecode(std::string_view url_encoded)
{
    std::string res;
    res.reserve(url_encoded.size());

    for (size_t i = 0; i < url_encoded.size(); ++i) {
        char c = url_encoded[i];
        // Special handling for percent which should be followed by two hex digits
        // representing an octet values, see RFC 3986, Section 2.1 Percent-Encoding
        if (c == '%' && i + 2 < url_encoded.size()) {
            unsigned int decoded_value{0};
            auto [p, ec] = std::from_chars(url_encoded.data() + i + 1, url_encoded.data() + i + 3, decoded_value, 16);

            // Only if there is no error and the pointer is set to the end of
            // the string, we can be sure both characters were valid hex
            if (ec == std::errc{} && p == url_encoded.data() + i + 3) {
                res += static_cast<char>(decoded_value);
                // Next two characters are part of the percent encoding
                i += 2;
                continue;
            }
            // In case of invalid percent encoding, add the '%' and continue
        }
        res += c;
    }

    return res;
}
Repository: bitcoin/bitcoin
File: src/common/url.cpp (C++)

Code before:
// Copyright (c) 2015-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 <common/url.h>

#include <event2/http.h>

#include <cstdlib>
#include <string>

std::string urlDecode(const std::string &urlEncoded) {
    std::string res;
    if (!urlEncoded.empty()) {
        char *decoded = evhttp_uridecode(urlEncoded.c_str(), false, nullptr);
        if (decoded) {
            res = std::string(decoded);
            free(decoded);
        }
    }
    return res;
}


Review comment (suggestion):
nit: `std::errc` [is part of `system_error`](https://en.cppreference.com/w/cpp/error/errc)

Code after:
// Copyright (c) 2015-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 <common/url.h>

#include <charconv>
#include <string>
#include <string_view>
#include <system_error>

std::string UrlDecode(std::string_view url_encoded)
{
    std::string res;
    res.reserve(url_encoded.size());

    for (size_t i = 0; i < url_encoded.size(); ++i) {
        char c = url_encoded[i];
        // Special handling for percent which should be followed by two hex digits
        // representing an octet values, see RFC 3986, Section 2.1 Percent-Encoding
        if (c == '%' && i + 2 < url_encoded.size()) {
            unsigned int decoded_value{0};
            auto [p, ec] = std::from_chars(url_encoded.data() + i + 1, url_encoded.data() + i + 3, decoded_value, 16);

            // Only if there is no error and the pointer is set to the end of
            // the string, we can be sure both characters were valid hex
            if (ec == std::errc{} && p == url_encoded.data() + i + 3) {
                res += static_cast<char>(decoded_value);
                // Next two characters are part of the percent encoding
                i += 2;
                continue;
            }
            // In case of invalid percent encoding, add the '%' and continue
Repository: bitcoin/bitcoin
File: src/common/url.cpp (C++)

Code before:
// Copyright (c) 2015-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 <common/url.h>

#include <event2/http.h>

#include <cstdlib>
#include <string>

std::string urlDecode(const std::string &urlEncoded) {
    std::string res;
    if (!urlEncoded.empty()) {
        char *decoded = evhttp_uridecode(urlEncoded.c_str(), false, nullptr);
        if (decoded) {
            res = std::string(decoded);
            free(decoded);
        }
    }
    return res;
}


Review comment (bug):
I guess `i+3` fails, because you access the memory past the end of the string(view) as a reference and then take a pointer to that.

A possible fix could be to use iterators instead of memory-access. Something like this, but not sure if it compiles, or is the best solution.

```suggestion
            auto [_, ec] = std::from_chars(url_encoded.begin() + (i + 1), url_encoded.begin() + (i + 3), decoded_value, 16);
```

Code after:
// Copyright (c) 2015-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 <common/url.h>

#include <charconv>
#include <string>
#include <string_view>
#include <system_error>

std::string UrlDecode(std::string_view url_encoded)
{
    std::string res;
    res.reserve(url_encoded.size());

    for (size_t i = 0; i < url_encoded.size(); ++i) {
        char c = url_encoded[i];
        // Special handling for percent which should be followed by two hex digits
        // representing an octet values, see RFC 3986, Section 2.1 Percent-Encoding
        if (c == '%' && i + 2 < url_encoded.size()) {
            unsigned int decoded_value{0};
            auto [p, ec] = std::from_chars(url_encoded.data() + i + 1, url_encoded.data() + i + 3, decoded_value, 16);

            // Only if there is no error and the pointer is set to the end of
            // the string, we can be sure both characters were valid hex
            if (ec == std::errc{} && p == url_encoded.data() + i + 3) {
                res += static_cast<char>(decoded_value);
                // Next two characters are part of the percent encoding
                i += 2;
                continue;
            }
            // In case of invalid percent encoding, add the '%' and continue
        }
        res += c;
    }

    return res;
}
Repository: bitcoin/bitcoin
File: src/common/url.cpp (C++)

Code before:
// Copyright (c) 2015-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 <common/url.h>

#include <event2/http.h>

#include <cstdlib>
#include <string>

std::string urlDecode(const std::string &urlEncoded) {
    std::string res;
    if (!urlEncoded.empty()) {
        char *decoded = evhttp_uridecode(urlEncoded.c_str(), false, nullptr);
        if (decoded) {
            res = std::string(decoded);
            free(decoded);
        }
    }
    return res;
}


Review comment (bug):
```suggestion
            auto [_, ec] = std::from_chars(url_encoded.data() + i + 1, url_encoded.data() + i + 3, decoded_value, 16);
```

Probably a wrong push?

Code after:
// Copyright (c) 2015-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 <common/url.h>

#include <charconv>
#include <string>
#include <string_view>
#include <system_error>

std::string UrlDecode(std::string_view url_encoded)
{
    std::string res;
    res.reserve(url_encoded.size());

    for (size_t i = 0; i < url_encoded.size(); ++i) {
        char c = url_encoded[i];
        // Special handling for percent which should be followed by two hex digits
        // representing an octet values, see RFC 3986, Section 2.1 Percent-Encoding
        if (c == '%' && i + 2 < url_encoded.size()) {
            unsigned int decoded_value{0};
            auto [p, ec] = std::from_chars(url_encoded.data() + i + 1, url_encoded.data() + i + 3, decoded_value, 16);

            // Only if there is no error and the pointer is set to the end of
            // the string, we can be sure both characters were valid hex
            if (ec == std::errc{} && p == url_encoded.data() + i + 3) {
                res += static_cast<char>(decoded_value);
                // Next two characters are part of the percent encoding
                i += 2;
                continue;
            }
            // In case of invalid percent encoding, add the '%' and continue
        }
        res += c;
    }

    return res;
}