ronantakizawa/codereview-bench huggingface dataset:
Repository: envoyproxy/envoy
File: api/envoy/extensions/filters/network/dynamic_modules/v3/dynamic_modules.proto (Protocol Buffers)

Code before:
  // ``filter_name``, the module can decide which in-module filter implementation to use and
  // fine-tune the behavior of the filter.
  //
  // For example, if a module has two filter implementations, one for echo and one for rate
  // limiting, ``filter_name`` is used to choose either echo or rate limiting. The
  // ``filter_config`` can be used to configure the echo behavior or the rate limiting parameters.
  //
  // ``google.protobuf.Struct`` is serialized as JSON before passing it to the module.
  // ``google.protobuf.BytesValue`` and ``google.protobuf.StringValue`` are passed directly
  // without the wrapper.
  //
  // .. code-block:: yaml
  //
  //  # Passing a string value
  //  filter_config:
  //    "@type": "type.googleapis.com/google.protobuf.StringValue"
  //    value: hello
  //
  //  # Passing raw bytes
  //  filter_config:
  //    "@type": "type.googleapis.com/google.protobuf.BytesValue"
  //    value: aGVsbG8=  # echo -n "hello" | base64
  //
  google.protobuf.Any filter_config = 3;

  // If true, the dynamic module is a terminal filter to use without an upstream connection.
  //
  // The dynamic module is responsible for creating and sending the response to downstream.
  //
  // Defaults to ``false``.
  bool terminal_filter = 4;
}


Review comment (suggestion):
```suggestion
  // If ``true``, the dynamic module is a terminal filter to use without an upstream connection.
```

Code after:
  // ``filter_name``, the module can decide which in-module filter implementation to use and
  // fine-tune the behavior of the filter.
  //
  // For example, if a module has two filter implementations, one for echo and one for rate
  // limiting, ``filter_name`` is used to choose either echo or rate limiting. The
  // ``filter_config`` can be used to configure the echo behavior or the rate limiting parameters.
  //
  // ``google.protobuf.Struct`` is serialized as JSON before passing it to the module.
  // ``google.protobuf.BytesValue`` and ``google.protobuf.StringValue`` are passed directly
  // without the wrapper.
  //
  // .. code-block:: yaml
  //
  //  # Passing a string value
  //  filter_config:
  //    "@type": "type.googleapis.com/google.protobuf.StringValue"
  //    value: hello
  //
  //  # Passing raw bytes
  //  filter_config:
  //    "@type": "type.googleapis.com/google.protobuf.BytesValue"
  //    value: aGVsbG8=  # echo -n "hello" | base64
  //
  google.protobuf.Any filter_config = 3;

  // If ``true``, the dynamic module is a terminal filter to use without an upstream connection.
  //
  // The dynamic module is responsible for creating and sending the response to downstream.
  //
  // Defaults to ``false``.
  bool terminal_filter = 4;
}
Repository: envoyproxy/envoy
File: source/common/tls/cert_validator/utility.cc (C++)

Code before:
    if (err == X509_V_ERR_CERT_HAS_EXPIRED || err == X509_V_ERR_CERT_NOT_YET_VALID) {
      return 1;
    }
  }
  return ok;
}
} // namespace
#endif

void CertValidatorUtil::setIgnoreCertificateExpiration(X509_STORE_CTX* store_ctx) {
#if defined(X509_V_FLAG_NO_CHECK_TIME)
  X509_STORE_CTX_set_flags(store_ctx, X509_V_FLAG_NO_CHECK_TIME);
#else
  X509_STORE_CTX_set_verify_cb(store_ctx, ignoreCertificateExpirationCallback);
#endif
}

void CertValidatorUtil::setIgnoreCertificateExpiration(X509_STORE* store) {
#if defined(X509_V_FLAG_NO_CHECK_TIME)
  X509_STORE_set_flags(store, X509_V_FLAG_NO_CHECK_TIME);
#else
  X509_STORE_set_verify_cb(store, ignoreCertificateExpirationCallback);
#endif
}

std::vector<std::string> CertValidatorUtil::getCertificateSans(X509* cert) {
  std::vector<std::string> sans;
  // X509_get_ext_d2i should be available in all supported BoringSSL versions.
  bssl::UniquePtr<GENERAL_NAMES> san_names(
      static_cast<GENERAL_NAMES*>(X509_get_ext_d2i(cert, NID_subject_alt_name, nullptr, nullptr)));
  if (san_names != nullptr) {
    for (const GENERAL_NAME* general_name : san_names.get()) {
      sans.push_back(Utility::generalNameAsString(general_name));
    }
  }
  return sans;
}

} // namespace Tls
} // namespace TransportSockets
} // namespace Extensions
} // namespace Envoy


Review comment (security):
2 things:

* This should be in `source/common/tls/utility.cc` instead of here
* Please rename to `getCertificateSansForLogging`. Logging is the only time we want to discard the SAN-type (URI, DNS, etc). This name will discourage misuse, where misuse could be a security issue.

Code after:
    }
  }
  return ok;
}
} // namespace
#endif

void CertValidatorUtil::setIgnoreCertificateExpiration(X509_STORE_CTX* store_ctx) {
#if defined(X509_V_FLAG_NO_CHECK_TIME)
  X509_STORE_CTX_set_flags(store_ctx, X509_V_FLAG_NO_CHECK_TIME);
#else
  X509_STORE_CTX_set_verify_cb(store_ctx, ignoreCertificateExpirationCallback);
#endif
}

void CertValidatorUtil::setIgnoreCertificateExpiration(X509_STORE* store) {
#if defined(X509_V_FLAG_NO_CHECK_TIME)
  X509_STORE_set_flags(store, X509_V_FLAG_NO_CHECK_TIME);
#else
  X509_STORE_set_verify_cb(store, ignoreCertificateExpirationCallback);
#endif
}

} // namespace Tls
} // namespace TransportSockets
} // namespace Extensions
} // namespace Envoy
Repository: envoyproxy/envoy
File: source/common/upstream/locality_endpoint.h (C/C++)

Code before:
#pragma once

#include "envoy/config/core/v3/base.pb.h"
#include "envoy/config/endpoint/v3/endpoint_components.pb.h"

#include "source/common/protobuf/utility.h"

namespace Envoy {
namespace Upstream {

using LocalityEndpointTuple = std::tuple<const envoy::config::core::v3::Locality&,
                                         const envoy::config::endpoint::v3::LbEndpoint&>;
struct LocalityEndpointHash {
  size_t operator()(const LocalityEndpointTuple& values) const {
    const auto locality_hash = MessageUtil::hash(std::get<0>(values));
    const auto endpoint_hash = MessageUtil::hash(std::get<1>(values));
    return locality_hash ^ endpoint_hash;
  }
};

struct LocalityEndpointEqualTo {
  bool operator()(const LocalityEndpointTuple& lhs, const LocalityEndpointTuple& rhs) const {
    return MessageUtil::hash(std::get<0>(lhs)) == MessageUtil::hash(std::get<0>(rhs)) &&
           MessageUtil::hash(std::get<1>(lhs)) == MessageUtil::hash(std::get<1>(rhs));
  }
};

} // namespace Upstream
} // namespace Envoy


Review comment (suggestion):
emm, I don't think we need to change this, and this and above is used for 

`using HostsMap = absl::flat_hash_map<LocalityEndpointTuple, HostSharedPtr, LocalityEndpointHash,
                                     LocalityEndpointEqualTo>;`

it will calculate the hash and then use the equal if there is a need, which is fairly low possibility.


Code after:
#pragma once

#include "envoy/config/core/v3/base.pb.h"
#include "envoy/config/endpoint/v3/endpoint_components.pb.h"

#include "source/common/protobuf/utility.h"

namespace Envoy {
namespace Upstream {

using LocalityEndpointTuple = std::tuple<const envoy::config::core::v3::Locality&,
                                         const envoy::config::endpoint::v3::LbEndpoint&>;
struct LocalityEndpointHash {
  size_t operator()(const LocalityEndpointTuple& values) const {
    const auto locality_hash = MessageUtil::hash(std::get<0>(values));
    const auto endpoint_hash = MessageUtil::hash(std::get<1>(values));
    return locality_hash ^ endpoint_hash;
  }
};

struct LocalityEndpointEqualTo {
  bool operator()(const LocalityEndpointTuple& lhs, const LocalityEndpointTuple& rhs) const {
    return Protobuf::util::MessageDifferencer::Equals(std::get<0>(lhs), std::get<0>(rhs)) &&
           Protobuf::util::MessageDifferencer::Equals(std::get<1>(lhs), std::get<1>(rhs));
  }
};

} // namespace Upstream
} // namespace Envoy
Repository: envoyproxy/envoy
File: test/integration/utility.h (C/C++)

Code before:
    length_to_wait_for_ = 0;
    wait_for_length_ = false;

    if (timeout_timer->enabled()) {
      timeout_timer->disableTimer();
      return testing::AssertionSuccess();
    }

    return testing::AssertionFailure() << "Timed out waiting for " << length << " bytes of data\n";
  }

  const std::string& data() { return data_; }
  bool readLastByte() { return read_end_stream_; }
  void clearData(size_t count = std::string::npos) { data_.erase(0, count); }

private:
  Event::Dispatcher& dispatcher_;
  std::string data_to_wait_for_;
  std::string data_;
  bool exact_match_{true};
  bool read_end_stream_{};
  size_t length_to_wait_for_{0};
  bool wait_for_length_{false};
};

envoy::config::bootstrap::v3::Bootstrap configToBootstrap(const std::string& config);

} // namespace Envoy


Review comment (suggestion):
Can this be inserted into some class (as a static function)?
From the code above it can be defined in test/integration/http_integration.h (`HttpIntegrationTest`).

Code after:
    length_to_wait_for_ = 0;
    wait_for_length_ = false;

    if (timeout_timer->enabled()) {
      timeout_timer->disableTimer();
      return testing::AssertionSuccess();
    }

    return testing::AssertionFailure() << "Timed out waiting for " << length << " bytes of data\n";
  }

  const std::string& data() { return data_; }
  bool readLastByte() { return read_end_stream_; }
  void clearData(size_t count = std::string::npos) { data_.erase(0, count); }

private:
  Event::Dispatcher& dispatcher_;
  std::string data_to_wait_for_;
  std::string data_;
  bool exact_match_{true};
  bool read_end_stream_{};
  size_t length_to_wait_for_{0};
  bool wait_for_length_{false};
};

} // namespace Envoy
Repository: envoyproxy/envoy
File: api/envoy/admin/v3/server_info.proto (Protocol Buffers)

Code before:
    INITIALIZING = 3;
  }

  // Server version.
  string version = 1;

  // State of the server.
  State state = 2;

  // Uptime since current epoch was started.
  google.protobuf.Duration uptime_current_epoch = 3;

  // Uptime since the start of the first epoch.
  google.protobuf.Duration uptime_all_epochs = 4;

  // Hot restart version.
  string hot_restart_version = 5;

  // Command line options the server is currently running with.
  CommandLineOptions command_line_options = 6;

  // Populated node identity of this server.
  config.core.v3.Node node = 7;
}

// [#next-free-field: 43]
message CommandLineOptions {
  option (udpa.annotations.versioning).previous_message_type =
      "envoy.admin.v2alpha.CommandLineOptions";

  enum IpVersion {
    v4 = 0;
    v6 = 1;
  }

  enum Mode {
    // Validate configs and then serve traffic normally.
    Serve = 0;

    // Validate configs and exit.
    Validate = 1;

    // Completely load and initialize the config, and then exit without running the listener loop.
    InitOnly = 2;
  }

  enum DrainStrategy {
    // Gradually discourage connections over the course of the drain period.
    Gradual = 0;

    // Discourage all connections for the duration of the drain sequence.

Review comment (suggestion):
Rather than adding a special new field, will the INITIALIZING `state` + hot restart epoch number could also achieve similar target?

/wait-any

Code after:
    INITIALIZING = 3;
  }

  // Server version.
  string version = 1;

  // State of the server.
  State state = 2;

  // Uptime since current epoch was started.
  google.protobuf.Duration uptime_current_epoch = 3;

  // Uptime since the start of the first epoch.
  google.protobuf.Duration uptime_all_epochs = 4;

  // Hot restart version.
  string hot_restart_version = 5;

  // Command line options the server is currently running with.
  CommandLineOptions command_line_options = 6;

  // Populated node identity of this server.
  config.core.v3.Node node = 7;

  // Whether the server is currently initializing during a hot restart.
  bool hot_restart_initializing = 8;
}

// [#next-free-field: 43]
message CommandLineOptions {
  option (udpa.annotations.versioning).previous_message_type =
      "envoy.admin.v2alpha.CommandLineOptions";

  enum IpVersion {
    v4 = 0;
    v6 = 1;
  }

  enum Mode {
    // Validate configs and then serve traffic normally.
    Serve = 0;

    // Validate configs and exit.
    Validate = 1;

    // Completely load and initialize the config, and then exit without running the listener loop.
    InitOnly = 2;
  }

  enum DrainStrategy {
    // Gradually discourage connections over the course of the drain period.
Repository: envoyproxy/envoy
File: mobile/library/cc/engine.cc (C++)

Code before:
#include "engine.h"

#include "absl/status/status.h"
#include "absl/strings/string_view.h"
#include "library/common/internal_engine.h"
#include "library/common/types/c_types.h"

namespace Envoy {
namespace Platform {

absl::StatusOr<std::shared_ptr<Engine>>
Engine::createFromInternalEngineHandle(int64_t internal_engine_handle) {
  auto* internal_engine = reinterpret_cast<::Envoy::InternalEngine*>(internal_engine_handle);
  if (internal_engine == nullptr) {
    return absl::InvalidArgumentError("can't cast internal_engine_handle to an InternalEngine*");
  }
  return std::shared_ptr<Engine>(new Engine(internal_engine, /*owns_engine=*/false));
}

Engine::Engine(::Envoy::InternalEngine* engine, bool owns_engine)
    : engine_(engine), owns_engine_(owns_engine) {}

Engine::~Engine() {
  if (owns_engine_ && !engine_->isTerminated()) {
    terminate();
  }
}

// we lazily construct the stream and pulse clients
// because they either require or will require a weak ptr
// which can't be provided from inside of the constructor
// because of how std::enable_shared_from_this works
StreamClientSharedPtr Engine::streamClient() {
  return std::make_shared<StreamClient>(shared_from_this());
}

std::string Engine::dumpStats() { return engine_->dumpStats(); }

int64_t Engine::getInternalEngineHandle() const { return reinterpret_cast<int64_t>(engine_); }


Review comment (bug):
Literally reinterppret_cast can't fail. Maybe change the error message to "Invalid internal engine handle."

Code after:
#include "engine.h"

#include "absl/status/status.h"
#include "absl/strings/string_view.h"
#include "library/common/internal_engine.h"
#include "library/common/types/c_types.h"

namespace Envoy {
namespace Platform {

absl::StatusOr<std::shared_ptr<Engine>>
Engine::createFromInternalEngineHandle(int64_t internal_engine_handle) {
  auto* internal_engine = reinterpret_cast<::Envoy::InternalEngine*>(internal_engine_handle);
  if (internal_engine == nullptr) {
    return absl::InvalidArgumentError("Invalid internal engine handle.");
  }
  return std::shared_ptr<Engine>(new Engine(internal_engine, /*handle_termination=*/false));
}

Engine::Engine(::Envoy::InternalEngine* engine, bool handle_termination)
    : engine_(engine), handle_termination_(handle_termination) {}

Engine::~Engine() {
  if (handle_termination_ && !engine_->isTerminated()) {
    terminate();
  }
}

// we lazily construct the stream and pulse clients
// because they either require or will require a weak ptr
// which can't be provided from inside of the constructor
// because of how std::enable_shared_from_this works
StreamClientSharedPtr Engine::streamClient() {
  return std::make_shared<StreamClient>(shared_from_this());
}

std::string Engine::dumpStats() { return engine_->dumpStats(); }

int64_t Engine::getInternalEngineHandle() const { return reinterpret_cast<int64_t>(engine_); }
Repository: envoyproxy/envoy
File: test/mocks/server/hot_restart.cc (C++)

Code before:
#include "hot_restart.h"

#include <string>

#include "gmock/gmock.h"
#include "gtest/gtest.h"

namespace Envoy {
namespace Server {

using ::testing::_;
using ::testing::Return;
using ::testing::ReturnRef;

MockHotRestart::MockHotRestart() : stats_allocator_(*symbol_table_) {
  ON_CALL(*this, logLock()).WillByDefault(ReturnRef(log_lock_));
  ON_CALL(*this, accessLogLock()).WillByDefault(ReturnRef(access_log_lock_));
  ON_CALL(*this, statsAllocator()).WillByDefault(ReturnRef(stats_allocator_));
  ON_CALL(*this, duplicateParentListenSocket(_, _, _)).WillByDefault(Return(-1));
}

MockHotRestart::~MockHotRestart() = default;

} // namespace Server
} // namespace Envoy


Review comment (refactor):
```suggestion
  ON_CALL(*this, duplicateParentListenSocket).WillByDefault(Return(-1));
```
(Makes it so a similar change won't be necessary if there's a future change to the number of args.)

Code after:
#include "hot_restart.h"

#include <string>

#include "gmock/gmock.h"
#include "gtest/gtest.h"

namespace Envoy {
namespace Server {

using ::testing::_;
using ::testing::Return;
using ::testing::ReturnRef;

MockHotRestart::MockHotRestart() : stats_allocator_(*symbol_table_) {
  ON_CALL(*this, logLock()).WillByDefault(ReturnRef(log_lock_));
  ON_CALL(*this, accessLogLock()).WillByDefault(ReturnRef(access_log_lock_));
  ON_CALL(*this, statsAllocator()).WillByDefault(ReturnRef(stats_allocator_));
  ON_CALL(*this, duplicateParentListenSocket).WillByDefault(Return(-1));
}

MockHotRestart::~MockHotRestart() = default;

} // namespace Server
} // namespace Envoy
Repository: eslint/eslint
File: tests/lib/rules/no-empty-static-block.js (JavaScript)

Code before:
/**
 * @fileoverview Tests for no-empty-static-block rule.
 * @author Sosuke Suzuki
 */
"use strict";

//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const rule = require("../../../lib/rules/no-empty-static-block"),
    { RuleTester } = require("../../../lib/rule-tester");

//------------------------------------------------------------------------------
// Tests
//------------------------------------------------------------------------------

const ruleTester = new RuleTester();

ruleTester.run("no-empty-static-block", rule, {
    valid: [
        {
            code: "class Foo { static { bar(); } }",
            parserOptions: { ecmaVersion: 2022 }
        },
        {
            code: "class Foo { static { /* comments */ } }",
            parserOptions: { ecmaVersion: 2022 }
        },
        {
            code: "class Foo { static {\n// comment\n} }",
            parserOptions: { ecmaVersion: 2022 }
        },
        {
            code: "class Foo { static { bar(); } static { bar(); } }",
            parserOptions: { ecmaVersion: 2022 }
        }
    ],
    invalid: [
        {
            code: "class Foo { static {} }",
            parserOptions: { ecmaVersion: 2022 },
            errors: [{ messageId: "unexpected" }]

Review comment (suggestion):
nits: you can use `parserOptions: { ecmaVersion: 2022 }` here, so no longer need to write it in every case. :)

Code after:
/**
 * @fileoverview Tests for no-empty-static-block rule.
 * @author Sosuke Suzuki
 */
"use strict";

//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const rule = require("../../../lib/rules/no-empty-static-block"),
    { RuleTester } = require("../../../lib/rule-tester");

//------------------------------------------------------------------------------
// Tests
//------------------------------------------------------------------------------

const ruleTester = new RuleTester({
    parserOptions: { ecmaVersion: 2022 }
});

ruleTester.run("no-empty-static-block", rule, {
    valid: [
        "class Foo { static { bar(); } }",
        "class Foo { static { /* comments */ } }",
        "class Foo { static {\n// comment\n} }",
        "class Foo { static { bar(); } static { bar(); } }"
    ],
    invalid: [
        {
            code: "class Foo { static {} }",
            errors: [{ messageId: "unexpected" }]
        },
        {
            code: "class Foo { static { } }",
            errors: [{ messageId: "unexpected" }]
        },
        {
            code: "class Foo { static { \n\n } }",
            errors: [{ messageId: "unexpected" }]
        },
        {
            code: "class Foo { static { bar(); } static {} }",
Repository: eslint/eslint
File: lib/rules/no-empty-static-block.js (JavaScript)

Code before:
/**
 * @fileoverview Rule to disallow empty static blocks.
 * @author Sosuke Suzuki
 */
"use strict";

//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const astUtils = require("./utils/ast-utils");

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

/** @type {import('../shared/types').Rule} */
module.exports = {
    meta: {
        type: "suggestion",

        docs: {
            description: "Disallows empty static blocks",
            recommended: false,
            url: "https://eslint.org/docs/rules/no-empty-static-block"
        },

        schema: [],

        messages: {
            unexpected: "Unexpected empty static block."
        }
    },

    create(context) {
        const sourceCode = context.getSourceCode();

        return {
            StaticBlock(node) {
                const innerComments = sourceCode.getTokens(node, {
                    includeComments: true,
                    filter: astUtils.isCommentToken
                });

                if (node.body.length === 0 && innerComments.length === 0) {
                    context.report({
                        node,
                        loc: node.loc,

Review comment (suggestion):
```suggestion
            description: "Disallow empty static blocks",
```

Code after:
/**
 * @fileoverview Rule to disallow empty static blocks.
 * @author Sosuke Suzuki
 */
"use strict";

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

/** @type {import('../shared/types').Rule} */
module.exports = {
    meta: {
        type: "suggestion",

        docs: {
            description: "Disallow empty static blocks",
            recommended: false,
            url: "https://eslint.org/docs/rules/no-empty-static-block"
        },

        schema: [],

        messages: {
            unexpected: "Unexpected empty static block."
        }
    },

    create(context) {
        const sourceCode = context.getSourceCode();

        return {
            StaticBlock(node) {
                if (node.body.length === 0) {
                    const closingBrace = sourceCode.getLastToken(node);

                    if (sourceCode.getCommentsBefore(closingBrace).length === 0) {
                        context.report({
                            node,
                            messageId: "unexpected"
                        });
                    }
                }
            }
        };
    }
};