ronantakizawa/codereview-bench huggingface dataset:
Repository: caddyserver/caddy
File: modules/logging/filters.go (Go)

Code before:
}

func (m IPMaskFilter) mask(s string) string {
	var output strings.Builder
	for value := range strings.SplitSeq(s, ",") {
		value = strings.TrimSpace(value)
		host, port, err := net.SplitHostPort(value)
		if err != nil {
			host = value // assume whole thing was IP address
		}
		ipAddr := net.ParseIP(host)
		if ipAddr == nil {
			output.WriteString(value + ", ")
			continue
		}
		mask := m.v4Mask
		if ipAddr.To4() == nil {
			mask = m.v6Mask
		}
		masked := ipAddr.Mask(mask)
		if port == "" {
			output.WriteString(masked.String() + ", ")
			continue
		}

		output.WriteString(net.JoinHostPort(masked.String(), port) + ", ")
	}
	return strings.TrimSuffix(output.String(), ", ")
}

type filterAction string

const (
	// Replace value(s).
	replaceAction filterAction = "replace"

	// Hash value(s).
	hashAction filterAction = "hash"

	// Delete.
	deleteAction filterAction = "delete"
)

func (a filterAction) IsValid() error {
	switch a {
	case replaceAction, deleteAction, hashAction:
		return nil
	}

	return errors.New("invalid action type")
}

Review comment (suggestion):
```suggestion
		output.WriteString(net.JoinHostPort(masked.String(), port))
		output.WriteString(", ")
```

Code after:
}

func (m IPMaskFilter) mask(s string) string {
	parts := make([]string, 0)
	for value := range strings.SplitSeq(s, ",") {
		value = strings.TrimSpace(value)
		host, port, err := net.SplitHostPort(value)
		if err != nil {
			host = value // assume whole thing was IP address
		}
		ipAddr := net.ParseIP(host)
		if ipAddr == nil {
			parts = append(parts, value)
			continue
		}
		mask := m.v4Mask
		if ipAddr.To4() == nil {
			mask = m.v6Mask
		}
		masked := ipAddr.Mask(mask)
		if port == "" {
			parts = append(parts, masked.String())
			continue
		}

		parts = append(parts, net.JoinHostPort(masked.String(), port))
	}
	return strings.Join(parts, ", ")
}

type filterAction string

const (
	// Replace value(s).
	replaceAction filterAction = "replace"

	// Hash value(s).
	hashAction filterAction = "hash"

	// Delete.
	deleteAction filterAction = "delete"
)

func (a filterAction) IsValid() error {
	switch a {
	case replaceAction, deleteAction, hashAction:
		return nil
	}

	return errors.New("invalid action type")
}
Repository: caddyserver/caddy
File: modules/logging/filters.go (Go)

Code before:
		return net.CIDRMask(rawField, bitLen)
	}

	m.v4Mask = parseRawToMask(m.IPv4MaskRaw, 32)
	m.v6Mask = parseRawToMask(m.IPv6MaskRaw, 128)

	return nil
}

// Filter filters the input field.
func (m IPMaskFilter) Filter(in zapcore.Field) zapcore.Field {
	if array, ok := in.Interface.(caddyhttp.LoggableStringArray); ok {
		newArray := make(caddyhttp.LoggableStringArray, len(array))
		for i, s := range array {
			newArray[i] = m.mask(s)
		}
		in.Interface = newArray
	} else {
		in.String = m.mask(in.String)
	}

	return in
}

func (m IPMaskFilter) mask(s string) string {
	var output strings.Builder
	for value := range strings.SplitSeq(s, ",") {
		value = strings.TrimSpace(value)
		host, port, err := net.SplitHostPort(value)
		if err != nil {
			host = value // assume whole thing was IP address
		}
		ipAddr := net.ParseIP(host)
		if ipAddr == nil {
			output.WriteString(value + ", ")
			continue
		}
		mask := m.v4Mask
		if ipAddr.To4() == nil {
			mask = m.v6Mask
		}
		masked := ipAddr.Mask(mask)
		if port == "" {
			output.WriteString(masked.String() + ", ")
			continue
		}

		output.WriteString(net.JoinHostPort(masked.String(), port) + ", ")
	}
	return strings.TrimSuffix(output.String(), ", ")
}

Review comment (performance):
It would be more efficient to gather all parts in `[]string`, then join them using `strings.Join(s, ", ")`.

Code after:
		return net.CIDRMask(rawField, bitLen)
	}

	m.v4Mask = parseRawToMask(m.IPv4MaskRaw, 32)
	m.v6Mask = parseRawToMask(m.IPv6MaskRaw, 128)

	return nil
}

// Filter filters the input field.
func (m IPMaskFilter) Filter(in zapcore.Field) zapcore.Field {
	if array, ok := in.Interface.(caddyhttp.LoggableStringArray); ok {
		newArray := make(caddyhttp.LoggableStringArray, len(array))
		for i, s := range array {
			newArray[i] = m.mask(s)
		}
		in.Interface = newArray
	} else {
		in.String = m.mask(in.String)
	}

	return in
}

func (m IPMaskFilter) mask(s string) string {
	parts := make([]string, 0)
	for value := range strings.SplitSeq(s, ",") {
		value = strings.TrimSpace(value)
		host, port, err := net.SplitHostPort(value)
		if err != nil {
			host = value // assume whole thing was IP address
		}
		ipAddr := net.ParseIP(host)
		if ipAddr == nil {
			parts = append(parts, value)
			continue
		}
		mask := m.v4Mask
		if ipAddr.To4() == nil {
			mask = m.v6Mask
		}
		masked := ipAddr.Mask(mask)
		if port == "" {
			parts = append(parts, masked.String())
			continue
		}

		parts = append(parts, net.JoinHostPort(masked.String(), port))
	}
	return strings.Join(parts, ", ")
}
Repository: caddyserver/caddy
File: notify/notify_windows.go (Go)

Code before:
		accepts = svc.AcceptStop | svc.AcceptShutdown
	case "continue_pending":
		state = svc.ContinuePending
	case "pause_pending":
		state = svc.PausePending
	case "paused":
		state = svc.Paused
		accepts = svc.AcceptStop | svc.AcceptShutdown | svc.AcceptPauseAndContinue
	default:
		return fmt.Errorf("unknown status: %s", name)
	}

	globalStatus <- svc.Status{State: state, Accepts: accepts}
	return nil
}

// Error notifies the SCM that the service is stopping due to a failure,
// including a service-specific exit code. The returned error allows the
// caller to abort execution and let the service terminate.
func Error(err error, code int) error {
	if globalStatus != nil {
		globalStatus <- svc.Status{
			State:                   svc.StopPending,
			ServiceSpecificExitCode: uint32(code),
		}
		return fmt.Errorf("service failed (code=%d): %w", code, err)
	}

	return nil
}


Review comment (bug):
Align Windows with Linux semantics:

- Error should only indicate failure to notify the SCM, not wrap and return the original service error.
- Sending the status over the channel should return nil unless the notification itself fails.

This avoids misleading logs and preserves platform-consistent API expectations.


Code after:
		accepts = svc.AcceptStop | svc.AcceptShutdown
	case "continue_pending":
		state = svc.ContinuePending
	case "pause_pending":
		state = svc.PausePending
	case "paused":
		state = svc.Paused
		accepts = svc.AcceptStop | svc.AcceptShutdown | svc.AcceptPauseAndContinue
	default:
		log.Printf("unknown state: %s", name)
		return nil
	}

	globalStatus <- svc.Status{State: state, Accepts: accepts}
	return nil
}

// Error notifies the SCM that the service is stopping due to a failure,
// including a service-specific exit code.
func Error(err error, code int) error {
	if globalStatus != nil {
		globalStatus <- svc.Status{
			State:                   svc.StopPending,
			ServiceSpecificExitCode: uint32(code),
		}
	}

	return nil
}
Repository: caddyserver/caddy
File: notify/notify_windows.go (Go)

Code before:
	}
	return nil
}

// Stopping notifies the SCM that the service is in the process of stopping.
// This allows Windows to track the shutdown transition properly.
func Stopping() error {
	if globalStatus != nil {
		globalStatus <- svc.Status{State: svc.StopPending}
	}
	return nil
}

// Status sends an arbitrary service state to the SCM based on a string
// identifier. This allows higher-level code to map custom states to
// Windows service states when appropriate.
func Status(name string) error {
	if globalStatus == nil {
		return nil
	}

	var state svc.State
	var accepts svc.Accepted
	accepts = 0

	switch strings.ToLower(name) {
	case "stopped":
		state = svc.Stopped
	case "start_pending":
		state = svc.StartPending
	case "stop_pending":
		state = svc.StopPending
	case "running":
		state = svc.Running
		accepts = svc.AcceptStop | svc.AcceptShutdown
	case "continue_pending":
		state = svc.ContinuePending
	case "pause_pending":
		state = svc.PausePending
	case "paused":
		state = svc.Paused
		accepts = svc.AcceptStop | svc.AcceptShutdown | svc.AcceptPauseAndContinue
	default:
		return fmt.Errorf("unknown status: %s", name)
	}

	globalStatus <- svc.Status{State: state, Accepts: accepts}
	return nil
}

// Error notifies the SCM that the service is stopping due to a failure,

Review comment (bug):
On Linux, Status accepts any descriptive message.
This PR makes the Windows version fail for any unknown string, which creates unexpected cross-platform differences.
Unknown status names should be ignored (or logged) rather than returned as errors, and comments should clarify that Windows only supports a limited set of predefined states.

BTW, I think we could ask @mholt for his opinion and see if he has any other plans for Status.

Code after:
	}
	return nil
}

// Stopping notifies the SCM that the service is in the process of stopping.
// This allows Windows to track the shutdown transition properly.
func Stopping() error {
	if globalStatus != nil {
		globalStatus <- svc.Status{State: svc.StopPending}
	}
	return nil
}

// Status sends an arbitrary service state to the SCM based on a string
// identifier of [svc.State].
// The unknown states will be logged.
func Status(name string) error {
	if globalStatus == nil {
		return nil
	}

	var state svc.State
	var accepts svc.Accepted
	accepts = 0

	switch strings.ToLower(name) {
	case "stopped":
		state = svc.Stopped
	case "start_pending":
		state = svc.StartPending
	case "stop_pending":
		state = svc.StopPending
	case "running":
		state = svc.Running
		accepts = svc.AcceptStop | svc.AcceptShutdown
	case "continue_pending":
		state = svc.ContinuePending
	case "pause_pending":
		state = svc.PausePending
	case "paused":
		state = svc.Paused
		accepts = svc.AcceptStop | svc.AcceptShutdown | svc.AcceptPauseAndContinue
	default:
		log.Printf("unknown state: %s", name)
		return nil
	}

	globalStatus <- svc.Status{State: state, Accepts: accepts}
	return nil
}
Repository: caddyserver/caddy
File: notify/notify_windows.go (Go)

Code before:
// Copyright 2015 Matthew Holt and The Caddy Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package notify

import "golang.org/x/sys/windows/svc"

// globalStatus store windows service status, it can be
// use to notify caddy status.
var globalStatus chan<- svc.Status

func SetGlobalStatus(status chan<- svc.Status) {
	globalStatus = status
}

func Ready() error {
	if globalStatus != nil {
		globalStatus <- svc.Status{
			State:   svc.Running,
			Accepts: svc.AcceptStop | svc.AcceptShutdown,
		}
	}
	return nil
}

func Reloading() error {
	if globalStatus != nil {
		globalStatus <- svc.Status{State: svc.StartPending}
	}
	return nil
}

func Stopping() error {
	if globalStatus != nil {
		globalStatus <- svc.Status{State: svc.StopPending}
	}
	return nil
}

// TODO: not implemented
func Status(_ string) error { return nil }

// TODO: not implemented
func Error(_ error, _ int) error { return nil }


Review comment (suggestion):
It's best to use [`caddy.Log()`](https://pkg.go.dev/github.com/caddyserver/caddy/v2#Log)

Code after:
		return nil
	}

	var state svc.State
	var accepts svc.Accepted
	accepts = 0

	switch strings.ToLower(name) {
	case "stopped":
		state = svc.Stopped
	case "start_pending":
		state = svc.StartPending
	case "stop_pending":
		state = svc.StopPending
	case "running":
		state = svc.Running
		accepts = svc.AcceptStop | svc.AcceptShutdown
	case "continue_pending":
		state = svc.ContinuePending
	case "pause_pending":
		state = svc.PausePending
	case "paused":
		state = svc.Paused
		accepts = svc.AcceptStop | svc.AcceptShutdown | svc.AcceptPauseAndContinue
	default:
		log.Printf("unknown state: %s", name)
		return nil
	}

	globalStatus <- svc.Status{State: state, Accepts: accepts}
	return nil
}

// Error notifies the SCM that the service is stopping due to a failure,
// including a service-specific exit code.
func Error(err error, code int) error {
	if globalStatus != nil {
		globalStatus <- svc.Status{
			State:                   svc.StopPending,
			ServiceSpecificExitCode: uint32(code),
		}
	}

	return nil
}
Repository: carbon-language/carbon-lang
File: toolchain/sem_ir/stringify_type.cpp (C++)

Code before:
#include "toolchain/base/kind_switch.h"
#include "toolchain/sem_ir/entity_with_params_base.h"
#include "toolchain/sem_ir/ids.h"

namespace Carbon::SemIR {

// Map an instruction kind representing a type into an integer describing the
// precedence of that type's syntax. Higher numbers correspond to higher
// precedence.
static auto GetTypePrecedence(InstKind kind) -> int {
  CARBON_CHECK(kind.is_type() != InstIsType::Never,
               "Only called for kinds which can define a type.");
  if (kind == ConstType::Kind) {
    return -1;
  }
  if (kind == PointerType::Kind) {
    return -2;
  }
  return 0;
}

namespace {
// Contains the stack of steps for `StringifyTypeExpr`.
class StepStack {
 public:
  // The discriminator for the `Step` union.
  enum Kind : uint8_t {
    Inst,
    FixedString,
    ArrayBound,
    Name,
  };

  // An individual step in the stack.
  struct Step {
    // The kind of step to perform.
    Kind kind;

    union {
      // The instruction to print, when kind is Inst.
      InstId inst_id;
      // The fixed string to print, when kind is FixedString.
      const char* fixed_string;
      // The array bound to print, when kind is ArrayBound.
      InstId bound_id;
      // The name to print, when kind is Name.
      NameId name_id;
    };
  };

  // Starts a new stack, which always contains the first instruction to

Review comment (suggestion):
This may be out of scope, but I feel like this would be clearer as a member of `Step`, so it can be documented together with `kind`.

Code after:
#include "toolchain/base/kind_switch.h"
#include "toolchain/sem_ir/entity_with_params_base.h"
#include "toolchain/sem_ir/ids.h"

namespace Carbon::SemIR {

// Map an instruction kind representing a type into an integer describing the
// precedence of that type's syntax. Higher numbers correspond to higher
// precedence.
static auto GetTypePrecedence(InstKind kind) -> int {
  CARBON_CHECK(kind.is_type() != InstIsType::Never,
               "Only called for kinds which can define a type.");
  if (kind == ConstType::Kind) {
    return -1;
  }
  if (kind == PointerType::Kind) {
    return -2;
  }
  return 0;
}

namespace {
// Contains the stack of steps for `StringifyTypeExpr`.
class StepStack {
 public:
  // An individual step in the stack, which stringifies some component of a type
  // name.
  struct Step {
    // The kind of step to perform.
    enum Kind : uint8_t {
      Inst,
      FixedString,
      ArrayBound,
      Name,
    };

    Kind kind;

    union {
      // The instruction to print, when kind is Inst.
      InstId inst_id;
      // The fixed string to print, when kind is FixedString.
      const char* fixed_string;
      // The array bound to print, when kind is ArrayBound.
      InstId bound_id;
      // The name to print, when kind is Name.
      NameId name_id;
    };
  };

  // Starts a new stack, which always contains the first instruction to
Repository: carbon-language/carbon-lang
File: toolchain/sem_ir/stringify_type.cpp (C++)

Code before:
// precedence.
static auto GetTypePrecedence(InstKind kind) -> int {
  CARBON_CHECK(kind.is_type() != InstIsType::Never,
               "Only called for kinds which can define a type.");
  if (kind == ConstType::Kind) {
    return -1;
  }
  if (kind == PointerType::Kind) {
    return -2;
  }
  return 0;
}

namespace {
// Contains the stack of steps for `StringifyTypeExpr`.
class StepStack {
 public:
  // The discriminator for the `Step` union.
  enum Kind : uint8_t {
    Inst,
    FixedString,
    ArrayBound,
    Name,
  };

  // An individual step in the stack.
  struct Step {
    // The kind of step to perform.
    Kind kind;

    union {
      // The instruction to print, when kind is Inst.
      InstId inst_id;
      // The fixed string to print, when kind is FixedString.
      const char* fixed_string;
      // The array bound to print, when kind is ArrayBound.
      InstId bound_id;
      // The name to print, when kind is Name.
      NameId name_id;
    };
  };

  // Starts a new stack, which always contains the first instruction to
  // stringify.
  explicit StepStack(const SemIR::File* file, InstId outer_inst_id)
      : sem_ir_(file) {
    PushInstId(outer_inst_id);
  }

  // These push basic entries onto the stack.
  auto PushInstId(InstId inst_id) -> void {

Review comment (refactor):
This is all a bit abstract; I think it would help me to have more specifics about what a "step" represents. Maybe something like this?

```suggestion
  // An individual step in the stack, which prints some component of a type name.
```

(A more specific name like "PrintStep" might also help, but that may be out of scope here.)

Code after:
// precedence.
static auto GetTypePrecedence(InstKind kind) -> int {
  CARBON_CHECK(kind.is_type() != InstIsType::Never,
               "Only called for kinds which can define a type.");
  if (kind == ConstType::Kind) {
    return -1;
  }
  if (kind == PointerType::Kind) {
    return -2;
  }
  return 0;
}

namespace {
// Contains the stack of steps for `StringifyTypeExpr`.
class StepStack {
 public:
  // An individual step in the stack, which stringifies some component of a type
  // name.
  struct Step {
    // The kind of step to perform.
    enum Kind : uint8_t {
      Inst,
      FixedString,
      ArrayBound,
      Name,
    };

    Kind kind;

    union {
      // The instruction to print, when kind is Inst.
      InstId inst_id;
      // The fixed string to print, when kind is FixedString.
      const char* fixed_string;
      // The array bound to print, when kind is ArrayBound.
      InstId bound_id;
      // The name to print, when kind is Name.
      NameId name_id;
    };
  };

  // Starts a new stack, which always contains the first instruction to
  // stringify.
  explicit StepStack(const SemIR::File* file, InstId outer_inst_id)
      : sem_ir_(file) {
    PushInstId(outer_inst_id);
  }

  // These push basic entries onto the stack.
  auto PushInstId(InstId inst_id) -> void {
Repository: carbon-language/carbon-lang
File: common/check_internal.h (C/C++)

Code before:

// Implements the fatal macro.
//
// Similar to the check failure macro, but tags the message as a fatal one and
// leaves the stringified condition empty.
#define CARBON_INTERNAL_FATAL(...)                                  \
  (CARBON_INTERNAL_CHECK_IMPL##__VA_OPT__(_FORMAT)(                 \
       "FATAL", __FILE__, __LINE__, "" __VA_OPT__(, ) __VA_ARGS__), \
   CARBON_INTERNAL_FATAL_NORETURN_SUFFIX())

#ifdef NDEBUG
// For `DCHECK` in optimized builds we have a dead check that we want to
// potentially "use" arguments, but otherwise have the minimal overhead. We
// avoid forming interesting format strings here so that we don't have to
// repeatedly instantiate the `Check` function above. This format string would
// be an error if actually used.
#define CARBON_INTERNAL_DEAD_DCHECK(condition, ...) \
  CARBON_INTERNAL_DEAD_DCHECK_IMPL##__VA_OPT__(_FORMAT)(__VA_ARGS__)

#define CARBON_INTERNAL_DEAD_DCHECK_IMPL() \
  Carbon::Internal::CheckFail<"", "", 0, "", "">()

#define CARBON_INTERNAL_DEAD_DCHECK_IMPL_FORMAT(format_str, ...) \
  Carbon::Internal::CheckFail<"", "", 0, "", "">(__VA_ARGS__)

// The ChecKFail function itself is noreturn in NDEBUG.
#define CARBON_INTERNAL_FATAL_NORETURN_SUFFIX() void()
#else
#define CARBON_INTERNAL_FATAL_NORETURN_SUFFIX() std::abort()
#endif

#endif  // CARBON_COMMON_CHECK_INTERNAL_H_


Review comment (bug):
```suggestion
// The CheckFail function itself is noreturn in NDEBUG.
```

Code after:

// Implements the fatal macro.
//
// Similar to the check failure macro, but tags the message as a fatal one and
// leaves the stringified condition empty.
#define CARBON_INTERNAL_FATAL(...)                                  \
  (CARBON_INTERNAL_CHECK_IMPL##__VA_OPT__(_FORMAT)(                 \
       "FATAL", __FILE__, __LINE__, "" __VA_OPT__(, ) __VA_ARGS__), \
   CARBON_INTERNAL_FATAL_NORETURN_SUFFIX())

#ifdef NDEBUG
// For `DCHECK` in optimized builds we have a dead check that we want to
// potentially "use" arguments, but otherwise have the minimal overhead. We
// avoid forming interesting format strings here so that we don't have to
// repeatedly instantiate the `Check` function above. This format string would
// be an error if actually used.
#define CARBON_INTERNAL_DEAD_DCHECK(condition, ...) \
  CARBON_INTERNAL_DEAD_DCHECK_IMPL##__VA_OPT__(_FORMAT)(__VA_ARGS__)

#define CARBON_INTERNAL_DEAD_DCHECK_IMPL() \
  Carbon::Internal::CheckFail<"", "", 0, "", "">()

#define CARBON_INTERNAL_DEAD_DCHECK_IMPL_FORMAT(format_str, ...) \
  Carbon::Internal::CheckFail<"", "", 0, "", "">(__VA_ARGS__)

// The CheckFail function itself is noreturn in NDEBUG.
#define CARBON_INTERNAL_FATAL_NORETURN_SUFFIX() void()
#else
#define CARBON_INTERNAL_FATAL_NORETURN_SUFFIX() std::abort()
#endif

#endif  // CARBON_COMMON_CHECK_INTERNAL_H_