ronantakizawa/codereview-bench huggingface dataset:
Repository: dotnet/runtime
File: src/coreclr/jit/rangecheck.h (C/C++)

Code before:
    bool AddConstant(int i, bool* overflows = nullptr)
    {
        if (overflows)
        {
            *overflows = false;
        }
        switch (type)
        {
            case keDependent:
                return true;
            case keBinOpArray:
            case keConstant:
                if (IntAddOverflows(cns, i))
                {
                    if (overflows)
                    {
                        *overflows = true;
                    }
                    return false;
                }
                cns += i;
                return true;
            case keUndef:
            case keUnknown:
                // For these values of 'type', conservatively return false
                break;
        }

        return false;
    }
    bool MultiplyConstant(int i, bool* overflows = nullptr)
    {
        if (overflows)
        {
            *overflows = false;
        }
        switch (type)
        {
            case keDependent:
                return true;
            case keBinOpArray:
            case keConstant:
                if (CheckedOps::MulOverflows(cns, i, CheckedOps::Signed))
                {
                    if (overflows)
                    {
                        *overflows = true;
                    }
                    return false;
                }
                cns *= i;

Review comment (bug):
What is the difference between the return value and the `overflows` value? If we return false, doesn't it imply that it could also overflow?

Code after:
    bool IsBinOpArray() const
    {
        return type == keBinOpArray;
    }
    bool AddConstant(int i)
    {
        switch (type)
        {
            case keDependent:
                return true;
            case keBinOpArray:
            case keConstant:
                if (IntAddOverflows(cns, i))
                {
                    return false;
                }
                cns += i;
                return true;
            case keUndef:
            case keUnknown:
                // For these values of 'type', conservatively return false
                break;
        }

        return false;
    }

    bool Equals(const Limit& l) const
    {
        switch (type)
        {
            case keUndef:
            case keUnknown:
            case keDependent:
                return l.type == type;

            case keBinOpArray:
                return l.type == type && l.vn == vn && l.cns == cns;

            case keConstant:
                return l.type == type && l.cns == cns;
        }
        return false;
    }
#ifdef DEBUG
    const char* ToString(Compiler* comp) const
    {
        switch (type)
        {
            case keUndef:
                return "Undef";
Repository: dotnet/runtime
File: src/coreclr/jit/codegenwasm.cpp (C++)

Code before:

        case GT_NO_OP:
            instGen(INS_nop);
            break;

        case GT_CNS_INT:
        case GT_CNS_LNG:
        case GT_CNS_DBL:
            genCodeForConstant(treeNode);
            break;

        case GT_NEG:
        case GT_NOT:
            genCodeForNegNot(treeNode->AsOp());
            break;

        case GT_IND:
            genCodeForIndir(treeNode->AsIndir());
            break;

        case GT_STOREIND:
            genCodeForStoreInd(treeNode->AsStoreInd());
            break;

        case GT_PUTARG_REG:
            genPutArgReg(treeNode->AsOp());
            break;

        default:
#ifdef DEBUG
            NYIRAW(GenTree::OpName(treeNode->OperGet()));
#else
            NYI_WASM("Opcode not implemented");
#endif
            break;
    }
}

//------------------------------------------------------------------------
// genCodeForJTrue: emit Wasm br_if
//
// Arguments:
//    treeNode - predicate value
//
void CodeGen::genCodeForJTrue(GenTreeOp* jtrue)
{
    BasicBlock* const block = compiler->compCurBB;
    assert(block->KindIs(BBJ_COND));

    genConsumeOperands(jtrue);


Review comment (suggestion):
We should not produce `GT_PUTARG_REG` on WASM (it is useless, of course). This part of lowering should be put under `#ifdef HAS_FIXED_REGISTER_SET`.

Code after:
            break;

        case GT_NO_OP:
            instGen(INS_nop);
            break;

        case GT_CNS_INT:
        case GT_CNS_LNG:
        case GT_CNS_DBL:
            genCodeForConstant(treeNode);
            break;

        case GT_NEG:
        case GT_NOT:
            genCodeForNegNot(treeNode->AsOp());
            break;

        case GT_IND:
            genCodeForIndir(treeNode->AsIndir());
            break;

        case GT_STOREIND:
            genCodeForStoreInd(treeNode->AsStoreInd());
            break;

        default:
#ifdef DEBUG
            NYIRAW(GenTree::OpName(treeNode->OperGet()));
#else
            NYI_WASM("Opcode not implemented");
#endif
            break;
    }
}

//------------------------------------------------------------------------
// genCodeForJTrue: emit Wasm br_if
//
// Arguments:
//    treeNode - predicate value
//
void CodeGen::genCodeForJTrue(GenTreeOp* jtrue)
{
    BasicBlock* const block = compiler->compCurBB;
    assert(block->KindIs(BBJ_COND));

    genConsumeOperands(jtrue);

    BasicBlock* const trueTarget  = block->GetTrueTarget();
    BasicBlock* const falseTarget = block->GetFalseTarget();
Repository: dotnet/runtime
File: src/tests/Loader/classloader/ExtendedLayout/CStruct.cs (C#)

Code before:
[ExtendedLayout(ExtendedLayoutKind.CStruct)]
public struct NestedCStructType
{
    public int x;
}

[ExtendedLayout(ExtendedLayoutKind.CStruct)]
public struct CStructCustomCStructField
{
    public NestedCStructType y;
}

[StructLayout(LayoutKind.Sequential)]
public struct NestedSequentialType
{
    public int x;
}

[ExtendedLayout(ExtendedLayoutKind.CStruct)]
public struct CStructCustomSeqStructField
{
    public NestedSequentialType y;
}

public struct NestedAutoLayoutType
{
    public int x;
}

[ExtendedLayout(ExtendedLayoutKind.CStruct)]
public struct CStructCustomAutoStructField
{
    public NestedAutoLayoutType y;
}

[ExtendedLayout(ExtendedLayoutKind.CStruct)]
public struct EmptyCStruct
{
}

[ExtendedLayout(ExtendedLayoutKind.CStruct)]
public struct CStructWithOffsets
{
    [FieldOffset(4)]
    public int a;
}

[ExtendedLayout(ExtendedLayoutKind.CStruct)]
[StructLayout(LayoutKind.Auto, Size = 12)]
public struct CStructWithSize
{

Review comment (suggestion):
```suggestion
[StructLayout(LayoutKind.Auto)]
public struct NestedAutoLayoutType
```

Code after:
[ExtendedLayout(ExtendedLayoutKind.CStruct)]
public struct NestedCStructType
{
    public int x;
}

[ExtendedLayout(ExtendedLayoutKind.CStruct)]
public struct CStructCustomCStructField
{
    public NestedCStructType y;
}

[StructLayout(LayoutKind.Sequential)]
public struct NestedSequentialType
{
    public int x;
}

[ExtendedLayout(ExtendedLayoutKind.CStruct)]
public struct CStructCustomSeqStructField
{
    public NestedSequentialType y;
}

[StructLayout(LayoutKind.Auto)]
public struct NestedAutoLayoutType
{
    public int x;
}

[ExtendedLayout(ExtendedLayoutKind.CStruct)]
public struct CStructCustomAutoStructField
{
    public NestedAutoLayoutType y;
}

[ExtendedLayout(ExtendedLayoutKind.CStruct)]
public struct EmptyCStruct
{
}

[ExtendedLayout(ExtendedLayoutKind.CStruct)]
public ref struct CStructByRefLike
{
    public int a;
}
Repository: dotnet/runtime
File: src/tests/Loader/classloader/ExtendedLayout/CUnion.cs (C#)

Code before:
[ExtendedLayout(ExtendedLayoutKind.CUnion)]
public struct EmptyCUnion
{
}

[ExtendedLayout(ExtendedLayoutKind.CUnion)]
public struct CUnionWithOffsets
{
    [FieldOffset(4)]
    public int a;
}

[ExtendedLayout(ExtendedLayoutKind.CUnion)]
[StructLayout(LayoutKind.Auto, Size = 12)]
public struct CUnionWithSize
{
    public int a;
}

[ExtendedLayout(ExtendedLayoutKind.CUnion)]
[StructLayout(LayoutKind.Auto, Pack = 1)]
public struct CUnionWithPack
{
    public byte a;
    public int b;
}

[ExtendedLayout(ExtendedLayoutKind.CUnion)]
public struct CUnionMixedSizes
{
    public byte byteField;
    public short shortField;
    public int intField;
    public long longField;
}

[ExtendedLayout(ExtendedLayoutKind.CUnion)]
public struct CUnionTwoInts
{
    public int first;
    public int second;
}

[ExtendedLayout(ExtendedLayoutKind.CUnion)]
public ref struct CUnionByRefLike
{
    public int a;
}


Review comment (suggestion):
Please keep types that require both `StructLayout` and `ExtendedLayout` when written in C# in IL.

Code after:
[ExtendedLayout(ExtendedLayoutKind.CUnion)]
public struct EmptyCUnion
{
}

[ExtendedLayout(ExtendedLayoutKind.CUnion)]
public struct CUnionMixedSizes
{
    public byte byteField;
    public short shortField;
    public int intField;
    public long longField;
}

[ExtendedLayout(ExtendedLayoutKind.CUnion)]
public struct CUnionTwoInts
{
    public int first;
    public int second;
}

[ExtendedLayout(ExtendedLayoutKind.CUnion)]
public ref struct CUnionByRefLike
{
    public int a;
}
Repository: dotnet/runtime
File: src/tests/Regressions/coreclr/GitHub_122933/test122933.cs (C#)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Linq;
using System.Reflection;
using Xunit;

// Small repro for ilasm/ildasm roundtrip issue with duplicated constraints
// This demonstrates the issue where constraints get duplicated during ilasm roundtrip
public interface IComp<TSelf>
    : IEq<TSelf>
        where TSelf : IComp<TSelf>?
{
}

public interface IEq<TSelf>
        where TSelf : IEq<TSelf>?
{
}

public class Test122933
{
    [Fact]
    public static int TestEntryPoint()
    {
        Type ieqType = typeof(IEq<>);
        Type icompType = typeof(IComp<>);

        Type[] ieqTypeParams = ieqType.GetGenericArguments();
        if (ieqTypeParams.Length != 1)
        {
            Console.WriteLine($"FAIL: IEq should have 1 generic parameter, but has {ieqTypeParams.Length}");
            return -1;
        }

Review comment (refactor):
```suggestion
// Validates that ilasm/ildasm roundtrip does not duplicated self-referential constraints

```

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Reflection;
using Xunit;

// Validates that ilasm/ildasm roundtrip does not duplicate self-referential constraints

public interface IComp<TSelf>
    : IEq<TSelf>
        where TSelf : IComp<TSelf>?
{
}

public interface IEq<TSelf>
        where TSelf : IEq<TSelf>?
{
}

public class Test122933
{
    [Fact]
    public static int TestEntryPoint()
    {
        Type ieqType = typeof(IEq<>);
        Type icompType = typeof(IComp<>);

        Type[] ieqTypeParams = ieqType.GetGenericArguments();
        if (ieqTypeParams.Length != 1)
        {
            Console.WriteLine($"FAIL: IEq should have 1 generic parameter, but has {ieqTypeParams.Length}");
            return -1;
        }
        Type ieqTSelf = ieqTypeParams[0];
Repository: dotnet/runtime
File: src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs (C#)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using Microsoft.Extensions.DependencyInjection.Specification.Fakes;
using Xunit;
using Xunit.Abstractions;
using Xunit.Sdk;

namespace Microsoft.Extensions.DependencyInjection.ServiceLookup
{
    public class ServiceProviderEngineScopeTests
    {
        [Fact]
        public void DoubleDisposeWorks()
        {
            var provider = new ServiceProvider(new ServiceCollection(), ServiceProviderOptions.Default);
            var serviceProviderEngineScope = new ServiceProviderEngineScope(provider, isRootScope: true);
            serviceProviderEngineScope.ResolvedServices.Add(new ServiceCacheKey(ServiceIdentifier.FromServiceType(typeof(IFakeService)), 0), null);
            serviceProviderEngineScope.Dispose();
            serviceProviderEngineScope.Dispose();
        }

        [Fact]
        public void RootEngineScopeDisposeTest()
        {
            var services = new ServiceCollection();
            ServiceProvider sp = services.BuildServiceProvider();
            var s = sp.GetRequiredService<IServiceProvider>();
            ((IDisposable)s).Dispose();

            Assert.Throws<ObjectDisposedException>(() => sp.GetRequiredService<IServiceProvider>());
        }

Review comment (suggestion):
The using directive for Xunit.Sdk is unused and should be removed.
```suggestion

```

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Threading.Tasks;
using Microsoft.Extensions.DependencyInjection.Specification.Fakes;
using Xunit;

namespace Microsoft.Extensions.DependencyInjection.ServiceLookup
{
    public class ServiceProviderEngineScopeTests
    {
        [Fact]
        public void DoubleDisposeWorks()
        {
            var provider = new ServiceProvider(new ServiceCollection(), ServiceProviderOptions.Default);
            var serviceProviderEngineScope = new ServiceProviderEngineScope(provider, isRootScope: true);
            serviceProviderEngineScope.ResolvedServices.Add(new ServiceCacheKey(ServiceIdentifier.FromServiceType(typeof(IFakeService)), 0), null);
            serviceProviderEngineScope.Dispose();
            serviceProviderEngineScope.Dispose();
        }

        [Fact]
        public void RootEngineScopeDisposeTest()
        {
            var services = new ServiceCollection();
            ServiceProvider sp = services.BuildServiceProvider();
            var s = sp.GetRequiredService<IServiceProvider>();
            ((IDisposable)s).Dispose();

            Assert.Throws<ObjectDisposedException>(() => sp.GetRequiredService<IServiceProvider>());
        }

        [Fact]
        public void ServiceProviderEngineScope_ImplementsAllServiceProviderInterfaces()
Repository: dotnet/runtime
File: src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs (C#)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Net.NetworkInformation;
using System.Runtime.ExceptionServices;
using System.Threading.Tasks;
using Microsoft.Extensions.Internal;

namespace Microsoft.Extensions.DependencyInjection.ServiceLookup
{
    [DebuggerDisplay("{DebuggerToString(),nq}")]
    [DebuggerTypeProxy(typeof(ServiceProviderEngineScopeDebugView))]
    internal sealed class ServiceProviderEngineScope : IServiceScope, IServiceProvider, IKeyedServiceProvider, IAsyncDisposable, IServiceScopeFactory
    {
        // For testing and debugging only
        internal IList<object> Disposables => _disposables ?? (IList<object>)Array.Empty<object>();

        private bool _disposed;
        private List<object>? _disposables;

        public ServiceProviderEngineScope(ServiceProvider provider, bool isRootScope)
        {
            ResolvedServices = new Dictionary<ServiceCacheKey, object?>();
            RootProvider = provider;
            IsRootScope = isRootScope;
        }

        internal Dictionary<ServiceCacheKey, object?> ResolvedServices { get; }


Review comment (suggestion):
`using System.Net.NetworkInformation;` is not used anywhere in this file and should be removed to keep the usings tidy and avoid unnecessary dependencies.
```suggestion

```

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.ExceptionServices;
using System.Threading.Tasks;
using Microsoft.Extensions.Internal;

namespace Microsoft.Extensions.DependencyInjection.ServiceLookup
{
    [DebuggerDisplay("{DebuggerToString(),nq}")]
    [DebuggerTypeProxy(typeof(ServiceProviderEngineScopeDebugView))]
    internal sealed class ServiceProviderEngineScope : IServiceScope, IServiceProvider, IKeyedServiceProvider, IAsyncDisposable, IServiceScopeFactory
    {
        // For testing and debugging only
        internal IList<object> Disposables => _disposables ?? (IList<object>)Array.Empty<object>();

        private bool _disposed;
        private List<object>? _disposables;

        public ServiceProviderEngineScope(ServiceProvider provider, bool isRootScope)
        {
            ResolvedServices = new Dictionary<ServiceCacheKey, object?>();
            RootProvider = provider;
            IsRootScope = isRootScope;
        }

        internal Dictionary<ServiceCacheKey, object?> ResolvedServices { get; }

        internal bool Disposed => _disposed;
Repository: dotnet/runtime
File: src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderEngineScopeTests.cs (C#)

Code before:
                _synchronous = synchronous;
            }

            public void Dispose()
            {
                IsDisposed = true;

                if (_throwsOnDispose)
                {
                    throw new InvalidOperationException(ErrorMessage);
                }
            }

            public ValueTask DisposeAsync()
            {
                if (_synchronous)
                {
                    Dispose();
                    return default;
                }

                return new ValueTask(DisposeAsyncInternal());

                async Task DisposeAsyncInternal()
                {
                    await Task.Delay(100);
                    Dispose();
                }
            }
        }
    }
}


Review comment (performance):
The 100ms delay in `DisposeAsyncInternal` slows these tests unnecessarily and will add up across multiple runs; since the goal is just to ensure the async disposal path is exercised (i.e. `DisposeAsync` completes asynchronously), this can be achieved with a much shorter delay or something like `Task.Yield` instead, which keeps the tests fast while still validating async behavior.
```suggestion
                    await Task.Yield();
```

Code after:
                _synchronous = synchronous;
            }

            public void Dispose()
            {
                IsDisposed = true;

                if (_throwsOnDispose)
                {
                    throw new InvalidOperationException(ErrorMessage);
                }
            }

            public ValueTask DisposeAsync()
            {
                if (_synchronous)
                {
                    Dispose();
                    return default;
                }

                return new ValueTask(DisposeAsyncInternal());

                async Task DisposeAsyncInternal()
                {
                    await Task.Yield();
                    Dispose();
                }
            }
        }
    }
}
Repository: dotnet/runtime
File: src/mono/wasm/testassets/BlazorWebWasm/BlazorWebWasm/Program.cs (C#)

Code before:

var builder = WebApplication.CreateBuilder(args);

// Add services to the container.
builder.Services.AddRazorComponents()
    .AddInteractiveWebAssemblyComponents();

var app = builder.Build();

var requestLogs = new List<BlazorWebWasmRequestLog>();

app.Use(async (context, next) =>
{
    await next.Invoke();
    var logEntry = new BlazorWebWasmRequestLog(
        DateTime.Now,
        context.Request.Method,
        context.Request.Path,
        context.Response.StatusCode
    );
    requestLogs.Add(logEntry);
});

// Configure the HTTP request pipeline.
if (app.Environment.IsDevelopment())
{
    app.UseWebAssemblyDebugging();
}
else
{
    app.UseExceptionHandler("/Error", createScopeForErrors: true);
    // The default HSTS value is 30 days. You may want to change this for production scenarios, see https://aka.ms/aspnetcore-hsts.
    app.UseHsts();
}
app.UseStatusCodePagesWithReExecute("/not-found", createScopeForStatusCodePages: true);
app.UseHttpsRedirection();

app.UseAntiforgery();

app.MapStaticAssets();
app.MapRazorComponents<App>()
    .AddInteractiveWebAssemblyRenderMode()
    .AddAdditionalAssemblies(typeof(BlazorWebWasm.Client._Imports).Assembly);

app.MapGet("/request-logs", () => requestLogs);
app.MapDelete("/request-logs", () => requestLogs.Clear());

app.Run();


Review comment (bug):
The requestLogs list is not thread-safe and can be accessed concurrently by multiple HTTP requests in the middleware. This can lead to race conditions. Consider using a thread-safe collection such as ConcurrentBag<BlazorWebWasmRequestLog> or adding proper synchronization.

Code after:

var builder = WebApplication.CreateBuilder(args);

// Add services to the container.
builder.Services.AddRazorComponents()
    .AddInteractiveWebAssemblyComponents();

var app = builder.Build();

var requestLogs = new List<BlazorWebWasmRequestLog>();
var requestLogsLock = new Lock();

app.Use(async (context, next) =>
{
    await next.Invoke();
    var logEntry = new BlazorWebWasmRequestLog(
        DateTime.UtcNow,
        context.Request.Method,
        context.Request.Path,
        context.Response.StatusCode
    );
    lock (requestLogsLock)
    {
        requestLogs.Add(logEntry);
    }
});

// Configure the HTTP request pipeline.
if (app.Environment.IsDevelopment())
{
    app.UseWebAssemblyDebugging();
}
else
{
    app.UseExceptionHandler("/Error", createScopeForErrors: true);
    // The default HSTS value is 30 days. You may want to change this for production scenarios, see https://aka.ms/aspnetcore-hsts.
    app.UseHsts();
}
app.UseStatusCodePagesWithReExecute("/not-found", createScopeForStatusCodePages: true);
app.UseHttpsRedirection();

app.UseAntiforgery();

app.MapStaticAssets();
app.MapRazorComponents<App>()
    .AddInteractiveWebAssemblyRenderMode()
    .AddAdditionalAssemblies(typeof(BlazorWebWasm.Client._Imports).Assembly);

app.MapGet("/request-logs", () =>
{
    lock (requestLogsLock)