ronantakizawa/codereview-bench huggingface dataset:
Repository: gabime/spdlog
File: include/spdlog/tcp_sink.h (C/C++)

Code before:

#include <spdlog/spdlog.h>
#include <spdlog/sinks/base_sink.h>
#include <sys/socket.h>
#include <arpa/inet.h>
#include <unistd.h>
using namespace std;
template<typename Mutex>
class tcp_sink : public spdlog::sinks::base_sink <Mutex>
{
public:
    tcp_sink(std::string address,int port)
    {
        if ((sock = socket(AF_INET, SOCK_STREAM, 0)) < 0)
        {
            printf("\n Socket creation error \n");
        }
        serv_addr.sin_family = AF_INET;
        serv_addr.sin_port = htons(port);
        if(inet_pton(AF_INET, address.c_str(), &serv_addr.sin_addr)<=0)
        {
            printf("\nInvalid address/ Address not supported \n");
        }
        if (connect(sock, (struct sockaddr *)&serv_addr, sizeof(serv_addr)) < 0)
        {
            printf("\nConnection Failed \n");
        }

    }
protected:
    void sink_it_(const spdlog::details::log_msg& msg) override
    {
        spdlog::memory_buf_t formatted;
        spdlog::sinks::base_sink<Mutex>::formatter_->format(msg, formatted);
        std::string message = fmt::to_string(formatted);
        send(sock , message.c_str() , message.size() , 0 );
    }

    void flush_() override
    {
    }

Review comment (suggestion):
Please throw in case of errors
For example:

```c++
SPDLOG_THROW(spdlog_ex("Socket creation error", errno));
```

Code after:
// Copyright(c) 2015-present, Gabi Melman & spdlog contributors.
// Distributed under the MIT License (http://opensource.org/licenses/MIT)

#include <spdlog/spdlog.h>
#include <spdlog/sinks/base_sink.h>
#include <spdlog/common-inl.h>
#include <sys/socket.h>
#include <arpa/inet.h>
#include <unistd.h>

template<typename Mutex>
class tcp_sink : public spdlog::sinks::base_sink <Mutex>
{
public:
    tcp_sink(std::string address,int port)
    {
        if ((sock_ = socket(AF_INET, SOCK_STREAM, 0)) < 0)
        {
            SPDLOG_THROW(spdlog::spdlog_ex("Socket creation error", errno));
        }
        serv_addr_.sin_family = AF_INET;
        serv_addr_.sin_port = htons(port);
        if(inet_pton(AF_INET, address.c_str(), &serv_addr_.sin_addr)<=0)
        {
            SPDLOG_THROW(spdlog::spdlog_ex("Invalid address/ Address not supported", errno));
        }
        if (connect(sock_, (struct sockaddr *)&serv_addr_, sizeof(serv_addr_)) < 0)
        {
            SPDLOG_THROW(spdlog::spdlog_ex("Connection Failed", errno));
        }

    }
protected:
    void sink_it_(const spdlog::details::log_msg& msg) override
    {
        spdlog::memory_buf_t formatted;
        spdlog::sinks::base_sink<Mutex>::formatter_->format(msg, formatted);
        int res = send(sock_ , formatted.data() , formatted.size() , 0 );
        if(res < 0)
            SPDLOG_THROW(spdlog::spdlog_ex("Message Send Failed", errno));
    }
Repository: gabime/spdlog
File: include/spdlog/details/os-inl.h (C/C++)

Code before:
    if (str_size == 0)
    {
        target.resize(0);
        return;
    }

    int result_size = static_cast<int>(target.capacity());
    if ((str_size + 1) * 2 < result_size)
    {
        result_size = ::MultiByteToWideChar(CP_UTF8, MB_ERR_INVALID_CHARS, str.data(), str_size, NULL, 0);
    }

    if (result_size > 0)
    {
        target.resize(result_size);
        result_size = ::MultiByteToWideChar(CP_UTF8, MB_ERR_INVALID_CHARS, str.data(), str_size, (LPWSTR)target.data(), result_size);

        if (result_size > 0)
        {
            target.resize(result_size);
            return;
        }
    }

    throw_spdlog_ex(fmt::format("WideCharToMultiByte failed. Last error: {}", ::GetLastError()));
}
#endif // (defined(SPDLOG_WCHAR_TO_UTF8_SUPPORT) || defined(SPDLOG_WCHAR_FILENAMES)) && defined(_WIN32)

// return true on success
static SPDLOG_INLINE bool mkdir_(const filename_t &path)
{
#ifdef _WIN32
#ifdef SPDLOG_WCHAR_FILENAMES
    return ::_wmkdir(path.c_str()) == 0;
#else
    return ::_mkdir(path.c_str()) == 0;
#endif
#else
    return ::mkdir(path.c_str(), mode_t(0755)) == 0;
#endif
}

// create the given directory - and all directories leading to it
// return true on success or if the directory already exists
SPDLOG_INLINE bool create_dir(filename_t path)
{
    if (path_exists(path))
    {
        return true;
    }


Review comment (bug):
wrong error message in exception. should be “ MultiByteToWideChar”

Code after:
    if (str_size == 0)
    {
        target.resize(0);
        return;
    }

    int result_size = static_cast<int>(target.capacity());
    if ((str_size + 1) * 2 < result_size)
    {
        result_size = ::MultiByteToWideChar(CP_UTF8, MB_ERR_INVALID_CHARS, str.data(), str_size, NULL, 0);
    }

    if (result_size > 0)
    {
        target.resize(result_size);
        result_size = ::MultiByteToWideChar(CP_UTF8, MB_ERR_INVALID_CHARS, str.data(), str_size, (LPWSTR)target.data(), result_size);

        if (result_size > 0)
        {
            target.resize(result_size);
            return;
        }
    }

    throw_spdlog_ex(fmt::format("MultiByteToWideChar failed. Last error: {}", ::GetLastError()));
}
#endif // (defined(SPDLOG_WCHAR_TO_UTF8_SUPPORT) || defined(SPDLOG_WCHAR_FILENAMES)) && defined(_WIN32)

// return true on success
static SPDLOG_INLINE bool mkdir_(const filename_t &path)
{
#ifdef _WIN32
#ifdef SPDLOG_WCHAR_FILENAMES
    return ::_wmkdir(path.c_str()) == 0;
#else
    return ::_mkdir(path.c_str()) == 0;
#endif
#else
    return ::mkdir(path.c_str(), mode_t(0755)) == 0;
#endif
}

// create the given directory - and all directories leading to it
// return true on success or if the directory already exists
SPDLOG_INLINE bool create_dir(filename_t path)
{
    if (path_exists(path))
    {
        return true;
    }
Repository: gabime/spdlog
File: include/spdlog/details/fmt_helper.h (C/C++)

Code before:
#pragma once

#include <chrono>
#include <type_traits>
#include <iterator>
#include <spdlog/fmt/fmt.h>
#include <spdlog/common.h>

#ifdef SPDLOG_USE_STD_FORMAT
#    include <charconv>
#    include <limits>
#endif

// Some fmt helpers to efficiently format and pad ints and strings
namespace spdlog {
namespace details {
namespace fmt_helper {

inline spdlog::string_view_t to_string_view(const memory_buf_t &buf) SPDLOG_NOEXCEPT
{
    return spdlog::string_view_t{buf.data(), buf.size()};
}

#ifdef SPDLOG_USE_STD_FORMAT
template<typename T>
std::basic_string<T> to_string(const std::basic_string<T> &buf)
{
    return buf;
}
#else
template<typename T, size_t Size>
std::basic_string<T> to_string(const fmt::basic_memory_buffer<T, Size> &buf)
{
    return fmt::to_string(buf);
}
#endif

inline void append_string_view(spdlog::string_view_t view, memory_buf_t &dest)
{
    auto *buf_ptr = view.data();
    dest.append(buf_ptr, buf_ptr + view.size());
}

#ifdef SPDLOG_USE_STD_FORMAT
template<typename T>
inline void append_int(T n, memory_buf_t &dest)
{
    // Buffer should be large enough to hold all digits (digits10 + 1) and a sign
    SPDLOG_CONSTEXPR const auto BUF_SIZE = std::numeric_limits<T>::digits10 + 2;
    char buf[BUF_SIZE];


Review comment (performance):
maybe could be optimize by changing the signature to return reference

```cpp
std::basic_string<T> & to_string(const std::basic_string<T> &buf)
```

Code after:
#pragma once

#include <chrono>
#include <type_traits>
#include <iterator>
#include <spdlog/fmt/fmt.h>
#include <spdlog/common.h>

#ifdef SPDLOG_USE_STD_FORMAT
#    include <charconv>
#    include <limits>
#endif

// Some fmt helpers to efficiently format and pad ints and strings
namespace spdlog {
namespace details {
namespace fmt_helper {

inline spdlog::string_view_t to_string_view(const memory_buf_t &buf) SPDLOG_NOEXCEPT
{
    return spdlog::string_view_t{buf.data(), buf.size()};
}

inline void append_string_view(spdlog::string_view_t view, memory_buf_t &dest)
{
    auto *buf_ptr = view.data();
    dest.append(buf_ptr, buf_ptr + view.size());
}

#ifdef SPDLOG_USE_STD_FORMAT
template<typename T>
inline void append_int(T n, memory_buf_t &dest)
{
    // Buffer should be large enough to hold all digits (digits10 + 1) and a sign
    SPDLOG_CONSTEXPR const auto BUF_SIZE = std::numeric_limits<T>::digits10 + 2;
    char buf[BUF_SIZE];

    auto [ptr, ec] = std::to_chars(buf, buf + BUF_SIZE, n, 10);
    if (ec == std::errc())
    {
        dest.append(buf, ptr);
    }
    else
    {
        throw_spdlog_ex("Failed to format int", static_cast<int>(ec));
    }
}
#else
template<typename T>
inline void append_int(T n, memory_buf_t &dest)
{
Repository: gabime/spdlog
File: include/spdlog/mdc.h (C/C++)

Code before:
#include <spdlog/common.h>

namespace spdlog {

class SPDLOG_API mdc {
public:
    static void put(const std::string &key, const std::string &value) {
        get_context()[key] = value;
    }

    static std::string get(const std::string &key) {
        auto &context = get_context();
        auto it = context.find(key);
        if (it != context.end()) {
            return it->second;
        }
        return "";
    }

    static void remove(const std::string &key) { get_context().erase(key); }

    static void clear() { get_context().clear(); }

    static std::map<std::string, std::string> &get_context() {
        static thread_local std::map<std::string, std::string> context;
        return context;
    }
};

}  // namespace spdlog

Review comment (suggestion):
I think `get_context()` better be private(at least till we find out it is needed by users).

Code after:
#include <spdlog/common.h>
#include <map>

namespace spdlog {

class SPDLOG_API mdc {
public:
    static void put(const std::string &key, const std::string &value) {
        get_context()[key] = value;
    }

    static std::string get(const std::string &key) {
        auto &context = get_context();
        auto it = context.find(key);
        if (it != context.end()) {
            return it->second;
        }
        return "";
    }

    static void remove(const std::string &key) { get_context().erase(key); }

    static void clear() { get_context().clear(); }

    static std::map<std::string, std::string> &get_context() {
        static thread_local std::map<std::string, std::string> context;
        return context;
    }
};

}  // namespace spdlog
Repository: gabime/spdlog
File: include/spdlog/details/thread_pool-inl.h (C/C++)

Code before:
    q_.dequeue(incoming_async_msg);

    bool active = true;
    switch (incoming_async_msg.msg_type) {
        case async_msg_type::log: {
            incoming_async_msg.worker_ptr->backend_sink_it_(incoming_async_msg);
            active = true;
            break;
        }
        case async_msg_type::flush: {
            incoming_async_msg.worker_ptr->backend_flush_();
            active = true;
            break;
        }

        case async_msg_type::terminate: {
            active = false;
            break;
        }

        default: {
            assert(false);
        }
    }

    if (incoming_async_msg.promise) {
        incoming_async_msg.promise->set_value();
    }

    return active;
}

}  // namespace details
}  // namespace spdlog


Review comment (suggestion):
No need for all those changes. since promise only used in flush, it can be dealt directly in the switch case and not involve the rest of the code. Also for clarity I would call the struct member it async_msg.flush _promise since this is its prupose.

Code after:

    switch (incoming_async_msg.msg_type) {
        case async_msg_type::log: {
            incoming_async_msg.worker_ptr->backend_sink_it_(incoming_async_msg);
            return true;
        }
        case async_msg_type::flush: {
            incoming_async_msg.worker_ptr->backend_flush_();
            incoming_async_msg.flush_promise.set_value();
            return true;
        }

        case async_msg_type::terminate: {
            return false;
        }

        default: {
            assert(false);
        }
    }

    return true;
}

}  // namespace details
}  // namespace spdlog
Repository: gatsbyjs/gatsby
File: packages/gatsby/src/commands/develop.ts (TypeScript)

Code before:
let isRestarting

module.exports = async (program: IProgram): Promise<void> => {
  const developProcessPath = slash(require.resolve(`./develop-process`))
  // Run the actual develop server on a random port, and the proxy on the program port
  // which users will access
  const proxyPort = program.port
  const developPort = await getRandomPort()

  const developProcess = new ControllableScript(`
    const cmd = require("${developProcessPath}");
    const args = ${JSON.stringify({
      ...program,
      port: developPort,
      proxyPort,
    })};
    cmd(args);
  `)

  const proxy = startDevelopProxy({
    proxyPort: proxyPort,
    targetPort: developPort,
    programPath: program.directory,
  })

  await startGraphQLServer(program.directory, true)
  const statusServerPort = await getRandomPort()

  let unlock
  if (!isCI()) {
    unlock = await createServiceLock(program.directory, `developstatusserver`, {
      port: statusServerPort,
    })

    if (!unlock) {
      console.error(
        `Looks like develop for this site is already running. Try visiting http://localhost:8000/ maybe?`
      )
      process.exit(1)
    }
  }

  const statusServer = http.createServer().listen(statusServerPort)
  const io = socket(statusServer)

  const handleChildProcessIPC = (msg): void => {
    if (msg.type === `HEARTBEAT`) return
    if (process.send) {
      // Forward IPC
      process.send(msg)
    }

Review comment (suggestion):
I don't think develop should start recipes server, rather it should be the wrapper. You don't need to wait for develop to run. It will also allow us to not depend on `gatsby-recipes` in `gatsby`, which is a requirement.

Code after:

module.exports = async (program: IProgram): Promise<void> => {
  const developProcessPath = slash(require.resolve(`./develop-process`))
  // Run the actual develop server on a random port, and the proxy on the program port
  // which users will access
  const proxyPort = program.port
  const developPort = await getRandomPort()

  const developProcess = new ControllableScript(`
    const cmd = require("${developProcessPath}");
    const args = ${JSON.stringify({
      ...program,
      port: developPort,
      proxyPort,
    })};
    cmd(args);
  `)

  const proxy = startDevelopProxy({
    proxyPort: proxyPort,
    targetPort: developPort,
    programPath: program.directory,
  })

  const statusServerPort = await getRandomPort()

  let unlock
  if (!isCI()) {
    unlock = await createServiceLock(program.directory, `developstatusserver`, {
      port: statusServerPort,
    })

    if (!unlock) {
      console.error(
        `Looks like develop for this site is already running. Try visiting http://localhost:8000/ maybe?`
      )
      process.exit(1)
    }
  }

  const statusServer = http.createServer().listen(statusServerPort)
  const io = socket(statusServer)

  const handleChildProcessIPC = (msg): void => {
    if (msg.type === `HEARTBEAT`) return
    if (process.send) {
      // Forward IPC
      process.send(msg)
    }

    if (
Repository: gatsbyjs/gatsby
File: packages/gatsby-cli/src/recipes.ts (TypeScript)

Code before:
import { trackCli } from "gatsby-telemetry"
import execa from "execa"
import * as path from "path"
import * as fs from "fs"
import detectPort from "detect-port"

export async function recipesHandler(
  recipe: string | undefined
): Promise<void> {
  // We don't really care what port is used for GraphQL as it's
  // generally only for code to code communication or debugging.
  const graphqlPort = await detectPort(4000)
  trackCli(`RECIPE_RUN`, { name: recipe })

  // Start GraphQL serve
  const scriptPath = require.resolve(`gatsby-recipes/dist/graphql.js`)

  const subprocess = execa(`node`, [scriptPath, graphqlPort], {
    all: true,
    env: {
      // Chalk doesn't want to output color in a child process
      // as it (correctly) thinks it's not in a normal terminal environemnt.
      // Since we're just returning data, we'll override that.
      FORCE_COLOR: `true`,
    },
  })


Review comment (bug):
not really related to this PR but is there a way with the lock stuff to run a service in the foreground? Sometimes that's convenient to directly see the logs or to run the service with the node debugger.

Code after:
import { trackCli } from "gatsby-telemetry"
import runRecipe, { startGraphQLServer } from "gatsby-recipes"

export async function recipesHandler(
  projectRoot: string,
  recipe: string | undefined
): Promise<void> {
  trackCli(`RECIPE_RUN`, { name: recipe })

  const graphql = await startGraphQLServer(projectRoot)

  return runRecipe({
    recipe,
    graphqlPort: graphql.port,
    projectRoot,
  })
}
Repository: gatsbyjs/gatsby
File: packages/gatsby-cli/src/reporter/loggers/json/index.ts (TypeScript)

Code before:
import stripAnsi from "strip-ansi"
import _ from "lodash"

const isStringPayload = (action: ActionsUnion): action is ISetStatus =>
  typeof action.payload === `string`

const sanitizeAction = (action: ActionsUnion): ActionsUnion => {
  if (isStringPayload(action)) {
    return action
  }

  if (`text` in action.payload && action.payload.text) {
    action.payload.text = stripAnsi(action.payload.text)
  }
  if (`statusText` in action.payload && action.payload.statusText) {
    action.payload.statusText = stripAnsi(action.payload.statusText)
  }

  return action
}

onLogAction((action: ActionsUnion) => {
  const sanitizedAction = sanitizeAction(action)

  process.stdout.write(JSON.stringify(sanitizedAction) + `\n`)
})


Review comment (suggestion):
We actually want to change this module to not invoke functionality on import. So this `onLogAction` should be wrapped in an exported function, which should then be executed in the parent module. Mind making these changes?

```
export function initializeJSONLogger() {
  onLogAction(....)
}
```

Code after:
import stripAnsi from "strip-ansi"
import { cloneDeep } from "lodash"

const isStringPayload = (action: ActionsUnion): action is ISetStatus =>
  typeof action.payload === `string`

const sanitizeAction = (action: ActionsUnion): ActionsUnion => {
  const copiedAction = cloneDeep(action)

  if (isStringPayload(copiedAction)) {
    return copiedAction
  }

  if (`text` in copiedAction.payload && copiedAction.payload.text) {
    copiedAction.payload.text = stripAnsi(copiedAction.payload.text)
  }
  if (`statusText` in copiedAction.payload && copiedAction.payload.statusText) {
    copiedAction.payload.statusText = stripAnsi(copiedAction.payload.statusText)
  }

  return copiedAction
}

export function initializeJSONLogger(): void {
  onLogAction((action: ActionsUnion) => {
    const sanitizedAction = sanitizeAction(action)

    process.stdout.write(JSON.stringify(sanitizedAction) + `\n`)
  })
}
Repository: gatsbyjs/gatsby
File: packages/gatsby-cli/src/reporter/loggers/json/index.ts (TypeScript)

Code before:
import { onLogAction } from "../../redux/index"
import { ActionsUnion, ISetStatus } from "../../redux/types"
import stripAnsi from "strip-ansi"
import _ from "lodash"

const isStringPayload = (action: ActionsUnion): action is ISetStatus =>
  typeof action.payload === `string`

const sanitizeAction = (action: ActionsUnion): ActionsUnion => {
  if (isStringPayload(action)) {
    return action
  }

  if (`text` in action.payload && action.payload.text) {
    action.payload.text = stripAnsi(action.payload.text)
  }
  if (`statusText` in action.payload && action.payload.statusText) {
    action.payload.statusText = stripAnsi(action.payload.statusText)
  }

  return action
}

onLogAction((action: ActionsUnion) => {
  const sanitizedAction = sanitizeAction(action)

  process.stdout.write(JSON.stringify(sanitizedAction) + `\n`)
})


Review comment (suggestion):
I think there is a functional change going on here. In the previous version, we were creating a new object and spreading values into it. This had the benefit that we weren't mutating the underlying object.

I think there is a risk here that this function in it's new state would actually be modifying values that are in redux state (thanks JS object referencing.)

Let's change this to create a new object instead of modifying the one passed in.

Code after:
import { onLogAction } from "../../redux/index"
import { ActionsUnion, ISetStatus } from "../../redux/types"
import stripAnsi from "strip-ansi"
import { cloneDeep } from "lodash"

const isStringPayload = (action: ActionsUnion): action is ISetStatus =>
  typeof action.payload === `string`

const sanitizeAction = (action: ActionsUnion): ActionsUnion => {
  const copiedAction = cloneDeep(action)

  if (isStringPayload(copiedAction)) {
    return copiedAction
  }

  if (`text` in copiedAction.payload && copiedAction.payload.text) {
    copiedAction.payload.text = stripAnsi(copiedAction.payload.text)
  }
  if (`statusText` in copiedAction.payload && copiedAction.payload.statusText) {
    copiedAction.payload.statusText = stripAnsi(copiedAction.payload.statusText)
  }

  return copiedAction
}

export function initializeJSONLogger(): void {
  onLogAction((action: ActionsUnion) => {
    const sanitizedAction = sanitizeAction(action)

    process.stdout.write(JSON.stringify(sanitizedAction) + `\n`)
  })
}
Repository: gatsbyjs/gatsby
File: packages/gatsby-plugin-preact/src/gatsby-node.js (JavaScript)

Code before:
const PreactRefreshPlugin = require(`@prefresh/webpack`)

exports.onPreInit = () => {
  // force fast-refresh in gatsby
  process.env.GATSBY_HOT_LOADER = `fast-refresh`
}

exports.onCreateWebpackConfig = ({ stage, actions, getConfig }) => {
  const webpackPlugins = []
  if (stage === `develop`) {
    webpackPlugins.push(new PreactRefreshPlugin())

    // remove React refresh plugin, we want ot add preact refresh instead.
    const webpackConfig = getConfig()
    const reactRefreshPluginIndex = webpackConfig.plugins.findIndex(
      plugin => plugin.constructor.name === `ReactRefreshPlugin`
    )
    webpackConfig.plugins.splice(reactRefreshPluginIndex, 1)
    actions.replaceWebpackConfig(webpackConfig)

    // enable react-refresh babel plugin to enable hooks
    // @see https://github.com/JoviDeCroock/prefresh/tree/master/packages/webpack#using-hooks
    actions.setBabelPlugin({
      name: `react-refresh/babel`,
    })
  }

  actions.setWebpackConfig({
    resolve: {
      alias: {
        react: `preact/compat`,
        "react-dom": `preact/compat`,
      },
    },
    plugins: webpackPlugins,
  })
}


Review comment (suggestion):
small typo
```suggestion
    // remove React refresh plugin, we want to add preact refresh instead.
```

Code after:
const PreactRefreshPlugin = require(`@prefresh/webpack`)

exports.onPreInit = () => {
  // force fast-refresh in gatsby
  process.env.GATSBY_HOT_LOADER = `fast-refresh`
}

exports.onCreateWebpackConfig = ({ stage, actions, getConfig }) => {
  const webpackPlugins = []
  if (stage === `develop`) {
    webpackPlugins.push(new PreactRefreshPlugin())

    // remove React refresh plugin, we want to add preact refresh instead.
    const webpackConfig = getConfig()
    webpackConfig.plugins = webpackConfig.plugins.filter(
      plugin => plugin.constructor.name !== `ReactRefreshPlugin`
    )
    actions.replaceWebpackConfig(webpackConfig)

    // enable react-refresh babel plugin to enable hooks
    // @see https://github.com/JoviDeCroock/prefresh/tree/master/packages/webpack#using-hooks
    actions.setBabelPlugin({
      name: `react-refresh/babel`,
    })
  }

  actions.setWebpackConfig({
    resolve: {
      alias: {
        react: `preact/compat`,
        "react-dom": `preact/compat`,
      },
    },
    plugins: webpackPlugins,
  })
}