ronantakizawa/codereview-bench huggingface dataset:
Repository: dotnet/runtime
File: src/libraries/System.Private.CoreLib/src/System/Span.cs (C#)

Code before:
            if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)_length)
                ThrowHelper.ThrowArgumentOutOfRangeException();
#else
            if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start))
                ThrowHelper.ThrowArgumentOutOfRangeException();
#endif

            return new Span<T>(ref Unsafe.Add(ref _reference, (nint)(uint)start /* force zero-extension */), length);
        }

        /// <summary>
        /// Copies the contents of this span into a new array.  This heap
        /// allocates, so should generally be avoided, however it is sometimes
        /// necessary to bridge the gap with APIs written in terms of arrays.
        /// </summary>
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public T[] ToArray()
        {
            if (IsEmpty)
            {
                return Array.Empty<T>();
            }

            T[] array = new T[Length];
            CopyTo(array);
            return array;
        }
    }
}


Review comment (suggestion):
```suggestion
            var destination = new T[Length];
            CopyTo(destination);
            return destination;
```

Code after:
            if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)_length)
                ThrowHelper.ThrowArgumentOutOfRangeException();
#else
            if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start))
                ThrowHelper.ThrowArgumentOutOfRangeException();
#endif

            return new Span<T>(ref Unsafe.Add(ref _reference, (nint)(uint)start /* force zero-extension */), length);
        }

        /// <summary>
        /// Copies the contents of this span into a new array.  This heap
        /// allocates, so should generally be avoided, however it is sometimes
        /// necessary to bridge the gap with APIs written in terms of arrays.
        /// </summary>
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public T[] ToArray()
        {
            if (IsEmpty)
            {
                return Array.Empty<T>();
            }

            var destination = new T[Length];
            CopyTo(destination);
            return destination;
        }
    }
}
Repository: dotnet/runtime
File: src/libraries/Common/src/System/Collections/Generic/ValueListBuilder.cs (C#)

Code before:
            return _span.Slice(0, _pos);
        }

        public bool TryCopyTo(Span<T> destination, out int itemsWritten)
        {
            if (_span.Slice(0, _pos).TryCopyTo(destination))
            {
                itemsWritten = _pos;
                return true;
            }

            itemsWritten = 0;
            return false;
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public void Dispose()
        {
            int pos = _pos;
            T[]? toReturn = _arrayFromPool;

            this = default;

            if (toReturn != null)
            {
                _arrayFromPool = null;

#if SYSTEM_PRIVATE_CORELIB
                if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
                {
                    ArrayPool<T>.Shared.Return(toReturn, pos);
                }
                else
                {
                    ArrayPool<T>.Shared.Return(toReturn);
                }
#else
                if (!typeof(T).IsPrimitive)
                {
                    Array.Clear(toReturn, 0, pos);
                }

                ArrayPool<T>.Shared.Return(toReturn);
#endif
            }
        }

        /// <summary>
        /// Resize the internal buffer either by doubling current buffer size or
        /// by adding <paramref name="additionalCapacityBeyondPos"/> to
        /// <see cref="_pos"/> whichever is greater.

Review comment (suggestion):
`_arrayFromPool` is already null after `this = default`, so this would not be needed anymore.

Code after:
            return _span.Slice(0, _pos);
        }

        public bool TryCopyTo(Span<T> destination, out int itemsWritten)
        {
            if (_span.Slice(0, _pos).TryCopyTo(destination))
            {
                itemsWritten = _pos;
                return true;
            }

            itemsWritten = 0;
            return false;
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public void Dispose()
        {
            int pos = _pos;
            T[]? toReturn = _arrayFromPool;

            this = default;

            if (toReturn != null)
            {
#if SYSTEM_PRIVATE_CORELIB
                if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
                {
                    ArrayPool<T>.Shared.Return(toReturn, pos);
                }
                else
                {
                    ArrayPool<T>.Shared.Return(toReturn);
                }
#else
                if (!typeof(T).IsPrimitive)
                {
                    Array.Clear(toReturn, 0, pos);
                }

                ArrayPool<T>.Shared.Return(toReturn);
#endif
            }
        }

        /// <summary>
        /// Resize the internal buffer either by doubling current buffer size or
        /// by adding <paramref name="additionalCapacityBeyondPos"/> to
        /// <see cref="_pos"/> whichever is greater.
        /// </summary>
        /// <param name="additionalCapacityBeyondPos">
Repository: dotnet/runtime
File: src/libraries/System.DirectoryServices/src/System/DirectoryServices/ActiveDirectory/TrustHelper.cs (C#)

Code before:
                        policyHandle = Utils.GetPolicyHandle(serverName);
                    }
                    catch (UnauthorizedAccessException)
                    {
                        if (impersonated)
                        {
                            Utils.Revert();
                            impersonated = false;
                        }
                        // try anonymous
                        Utils.ImpersonateAnonymous();
                        impersonated = true;
                        policyHandle = Utils.GetPolicyHandle(serverName);
                    }

                    uint result = global::Interop.Advapi32.LsaQueryInformationPolicy(policyHandle.DangerousGetHandle(), policyDnsDomainInformation, ref buffer);
                    if (result != 0)
                    {
                        throw ExceptionHelper.GetExceptionFromErrorCode((int)global::Interop.Advapi32.LsaNtStatusToWinError(result), serverName);
                    }

                    return buffer;
                }
                finally
                {
                    policyHandle?.Dispose();
                        
                    if (impersonated)
                        Utils.Revert();
                }
            }
            catch { throw; }
        }
    }
}


Review comment (suggestion):
Took me a moment to see why this one is done differently from the others; it's because it's set in nested `try`/`catch` blocks.

Code after:
                        policyHandle = Utils.GetPolicyHandle(serverName);
                    }
                    catch (UnauthorizedAccessException)
                    {
                        if (impersonated)
                        {
                            Utils.Revert();
                            impersonated = false;
                        }
                        // try anonymous
                        Utils.ImpersonateAnonymous();
                        impersonated = true;
                        policyHandle = Utils.GetPolicyHandle(serverName);
                    }

                    uint result = global::Interop.Advapi32.LsaQueryInformationPolicy(policyHandle.DangerousGetHandle(), policyDnsDomainInformation, ref buffer);
                    if (result != 0)
                    {
                        throw ExceptionHelper.GetExceptionFromErrorCode((int)global::Interop.Advapi32.LsaNtStatusToWinError(result), serverName);
                    }

                    return buffer;
                }
                finally
                {
                    policyHandle?.Dispose();
                    if (impersonated)
                        Utils.Revert();
                }
            }
            catch { throw; }
        }
    }
}
Repository: dotnet/runtime
File: src/libraries/System.Runtime.InteropServices/tests/TestAssets/NativeExports/ComInterfaceGenerator/UniqueMarshalling.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.Runtime.InteropServices;
using System.Runtime.InteropServices.Marshalling;
using SharedTypes.ComInterfaces;

namespace NativeExports.ComInterfaceGenerator
{
    public static unsafe class UniqueMarshalling
    {
        // Call from another assembly to get a ptr to make an RCW
        [UnmanagedCallersOnly(EntryPoint = "new_unique_marshalling")]
        public static void* CreateComObject()
        {
            StrategyBasedComWrappers wrappers = new();
            var myObject = new SharedTypes.ComInterfaces.UniqueMarshalling();
            nint ptr = wrappers.GetOrCreateComInterfaceForObject(myObject, CreateComInterfaceFlags.None);

            return (void*)ptr;
        }
    }
}


Review comment (performance):
@copilot This method should cache the pointer and return the same pointer every time. There should be a test in LibraryImport.Tests analogous to the one in ComInterfaceGenerator.Tests that calls this twice and validates the object is different each time.

Code after:
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.InteropServices;
using System.Runtime.InteropServices.Marshalling;
using SharedTypes.ComInterfaces;

namespace NativeExports.ComInterfaceGenerator
{
    public static unsafe class UniqueMarshalling
    {
        private static void* s_cachedPtr = null;

        // Call from another assembly to get a ptr to make an RCW
        [UnmanagedCallersOnly(EntryPoint = "new_unique_marshalling")]
        public static void* CreateComObject()
        {
            if (s_cachedPtr == null)
            {
                StrategyBasedComWrappers wrappers = new();
                var myObject = new SharedTypes.ComInterfaces.UniqueMarshalling();
                nint ptr = wrappers.GetOrCreateComInterfaceForObject(myObject, CreateComInterfaceFlags.None);
                s_cachedPtr = (void*)ptr;
            }

            return s_cachedPtr;
        }
    }
}
Repository: dotnet/runtime
File: src/libraries/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Tests/NativeMarshallingAttributeTests.cs (C#)

Code before:

namespace ComInterfaceGenerator.Tests
{
    public unsafe partial class NativeMarshallingAttributeTests
    {
        [LibraryImport(NativeExportsNE.NativeExportsNE_Binary, EntryPoint = "new_unique_marshalling")]
        internal static partial IUniqueMarshalling NewUniqueMarshalling();

        [Fact]
        public void GetSameComInterfaceTwiceReturnsUniqueInstances()
        {
            // When using NativeMarshalling with UniqueComInterfaceMarshaller,
            // getting the same COM interface pointer twice should return different managed instances
            var obj1 = NewUniqueMarshalling();
            var obj2 = NewUniqueMarshalling();

            Assert.NotSame(obj1, obj2);

            // Both refer to the same underlying COM object (same cached pointer)
            obj1.SetValue(42);
            Assert.Equal(42, obj2.GetValue());

            // Modifying through one should affect the other
            obj2.SetValue(100);
            Assert.Equal(100, obj1.GetValue());
        }

        [Fact]
        public void MethodReturningComInterfaceReturnsUniqueInstance()
        {
            // When a COM interface method returns the same interface type,
            // it should return a new managed instance, not the cached one
            var obj = NewUniqueMarshalling();
            obj.SetValue(42);

            var returnedObj = obj.GetThis();

            // Should be a different managed object
            Assert.NotSame(obj, returnedObj);

            // But should refer to the same underlying COM object
            Assert.Equal(42, returnedObj.GetValue());

            // Modifying through one should affect the other
            returnedObj.SetValue(100);
            Assert.Equal(100, obj.GetValue());
        }
    }
}


Review comment (suggestion):
@copilot This is a good test, but it should go into a LibraryImports.Tests.csproj test since it tests the marshalling of a LibraryImport. Move it to that project instead.

Code after:

namespace ComInterfaceGenerator.Tests
{
    public unsafe partial class NativeMarshallingAttributeTests
    {
        [LibraryImport(NativeExportsNE.NativeExportsNE_Binary, EntryPoint = "new_unique_marshalling")]
        internal static partial IUniqueMarshalling NewUniqueMarshalling();

        [Fact]
        public void MethodReturningComInterfaceReturnsUniqueInstance()
        {
            // When a COM interface method returns the same interface type,
            // it should return a new managed instance, not the cached one
            var obj = NewUniqueMarshalling();
            obj.SetValue(42);

            var returnedObj = obj.GetThis();

            // Should be a different managed object
            Assert.NotSame(obj, returnedObj);

            // But should refer to the same underlying COM object
            Assert.Equal(42, returnedObj.GetValue());

            // Modifying through one should affect the other
            returnedObj.SetValue(100);
            Assert.Equal(100, obj.GetValue());
        }
    }
}
Repository: dotnet/runtime
File: src/native/libs/System.Globalization.Native/pal_icushim_static.c (C)

Code before:
    log_shim_error("ICU call %s failed with error #%d '%s'.", name, status, statusText);
}

#if defined(ICU_TRACING)
static void U_CALLCONV icu_trace_data(const void* context, int32_t fnNumber, int32_t level, const char* fmt, va_list args)
{
    char buf[1000];
    utrace_vformat(buf, sizeof(buf), 0, fmt, args);
    printf("[ICUDT] %s: %s\n", utrace_functionName(fnNumber), buf);
}
#endif

static int32_t load_icu_data(const void* pData);

#ifdef __EMSCRIPTEN__
#include <emscripten.h>

EMSCRIPTEN_KEEPALIVE int32_t mono_wasm_load_icu_data(const void* pData);
EMSCRIPTEN_KEEPALIVE int32_t coreclr_wasm_load_icu_data(const void* pData);

EMSCRIPTEN_KEEPALIVE int32_t mono_wasm_load_icu_data(const void* pData)
{
    return load_icu_data(pData);
}

EMSCRIPTEN_KEEPALIVE int32_t coreclr_wasm_load_icu_data(const void* pData)
{
    return load_icu_data(pData);
}

/*
 * driver.c calls this to make sure this file is linked, otherwise
 * its not, meaning the EMSCRIPTEN_KEEPALIVE functions above
 * are not kept.
 */
void mono_wasm_link_icu_shim(void);
void coreclr_wasm_link_icu_shim(void);

void mono_wasm_link_icu_shim(void)
{
}

void coreclr_wasm_link_icu_shim(void)
{
}

#endif

int32_t mono_wasi_load_icu_data(const void* pData);

int32_t mono_wasi_load_icu_data(const void* pData)

Review comment (suggestion):
please rename the `mono_wasm_load_icu_data` to `wasm_load_icu_data` and re-use it.


Code after:
    log_shim_error("ICU call %s failed with error #%d '%s'.", name, status, statusText);
}

#if defined(ICU_TRACING)
static void U_CALLCONV icu_trace_data(const void* context, int32_t fnNumber, int32_t level, const char* fmt, va_list args)
{
    char buf[1000];
    utrace_vformat(buf, sizeof(buf), 0, fmt, args);
    printf("[ICUDT] %s: %s\n", utrace_functionName(fnNumber), buf);
}
#endif

static int32_t load_icu_data(const void* pData);

#ifdef __EMSCRIPTEN__
#include <emscripten.h>

EMSCRIPTEN_KEEPALIVE int32_t wasm_load_icu_data(const void* pData);

EMSCRIPTEN_KEEPALIVE int32_t wasm_load_icu_data(const void* pData)
{
    return load_icu_data(pData);
}
/*
 * driver.c calls this to make sure this file is linked, otherwise
 * its not, meaning the EMSCRIPTEN_KEEPALIVE functions above
 * are not kept.
 */
void mono_wasm_link_icu_shim(void);

void mono_wasm_link_icu_shim(void)
{
}
#endif

int32_t mono_wasi_load_icu_data(const void* pData);

int32_t mono_wasi_load_icu_data(const void* pData)
{
    return load_icu_data(pData);
}

static int32_t load_icu_data(const void* pData)
{

    UErrorCode status = 0;
    udata_setCommonData(pData, &status);

    if (U_FAILURE(status))
    {
        log_icu_error("udata_setCommonData", status);
Repository: dotnet/runtime
File: src/native/libs/System.Globalization.Native/pal_icushim_static.c (C)

Code before:
EMSCRIPTEN_KEEPALIVE int32_t mono_wasm_load_icu_data(const void* pData);
EMSCRIPTEN_KEEPALIVE int32_t coreclr_wasm_load_icu_data(const void* pData);

EMSCRIPTEN_KEEPALIVE int32_t mono_wasm_load_icu_data(const void* pData)
{
    return load_icu_data(pData);
}

EMSCRIPTEN_KEEPALIVE int32_t coreclr_wasm_load_icu_data(const void* pData)
{
    return load_icu_data(pData);
}

/*
 * driver.c calls this to make sure this file is linked, otherwise
 * its not, meaning the EMSCRIPTEN_KEEPALIVE functions above
 * are not kept.
 */
void mono_wasm_link_icu_shim(void);
void coreclr_wasm_link_icu_shim(void);

void mono_wasm_link_icu_shim(void)
{
}

void coreclr_wasm_link_icu_shim(void)
{
}

#endif

int32_t mono_wasi_load_icu_data(const void* pData);

int32_t mono_wasi_load_icu_data(const void* pData)
{
    return load_icu_data(pData);
}

static int32_t load_icu_data(const void* pData)
{

    UErrorCode status = 0;
    udata_setCommonData(pData, &status);

    if (U_FAILURE(status))
    {
        log_icu_error("udata_setCommonData", status);
        return 0;
    }
    else
    {

Review comment (suggestion):
We can also rename/unify this.

Or even better, we could get rid of it. 
This is probably related to re-linking app with workload and invariant. In which case the code above should be trimmed.
Let's discuss if there is more elegant way of doing this. 
This should be a different PR.

Code after:
EMSCRIPTEN_KEEPALIVE int32_t wasm_load_icu_data(const void* pData);

EMSCRIPTEN_KEEPALIVE int32_t wasm_load_icu_data(const void* pData)
{
    return load_icu_data(pData);
}
/*
 * driver.c calls this to make sure this file is linked, otherwise
 * its not, meaning the EMSCRIPTEN_KEEPALIVE functions above
 * are not kept.
 */
void mono_wasm_link_icu_shim(void);

void mono_wasm_link_icu_shim(void)
{
}
#endif

int32_t mono_wasi_load_icu_data(const void* pData);

int32_t mono_wasi_load_icu_data(const void* pData)
{
    return load_icu_data(pData);
}

static int32_t load_icu_data(const void* pData)
{

    UErrorCode status = 0;
    udata_setCommonData(pData, &status);

    if (U_FAILURE(status))
    {
        log_icu_error("udata_setCommonData", status);
        return 0;
    }
    else
    {

#if defined(ICU_TRACING)
        // see https://github.com/unicode-org/icu/blob/master/docs/userguide/icu_data/tracing.md
        utrace_setFunctions(0, 0, 0, icu_trace_data);
        utrace_setLevel(UTRACE_VERBOSE);
#endif
        isDataSet = 1;
        return 1;
    }
}

static const char *
cstdlib_load_icu_data(const char *path)
Repository: dotnet/runtime
File: src/libraries/Common/src/Interop/Browser/Interop.Locale.CoreCLR.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.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

internal static partial class Interop
{
    internal static unsafe partial class JsGlobalization
    {
        [LibraryImport(Libraries.JavaScriptNative, EntryPoint = "SystemJS_GetLocaleInfo")]
        public static unsafe partial nint GetLocaleInfo(char* locale, int localeLength, char* culture, int cultureLength, char* buffer, int bufferLength, out int resultLength);
    }
}


Review comment (suggestion):
These using directives are unused and should be removed. The file only contains a `LibraryImport` attribute which doesn't require `System.Reflection` or `System.Runtime.CompilerServices`. Only `System` and `System.Runtime.InteropServices` are needed.
```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.Runtime.InteropServices;

internal static partial class Interop
{
    internal static unsafe partial class JsGlobalization
    {
        [LibraryImport(Libraries.JavaScriptNative, EntryPoint = "SystemJS_GetLocaleInfo")]
        public static unsafe partial nint GetLocaleInfo(char* locale, int localeLength, char* culture, int cultureLength, char* buffer, int bufferLength, out int resultLength);
    }
}
Repository: dotnet/runtime
File: src/coreclr/hosts/corerun/wasm/libCorerun.extpost.js (JavaScript)

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

/* eslint-disable no-undef */
/* eslint-disable space-before-function-paren */
/* eslint-disable @typescript-eslint/no-unused-vars */
var fetch = fetch || undefined; var dotnetNativeModuleLoaded = false; var dotnetInternals = null;
export function selfRun() {
    const Module = {};
    const corePreRun = () => {
        // copy all node/shell env variables to emscripten env
        if (globalThis.process && globalThis.process.env) {
            for (const [key, value] of Object.entries(process.env)) {
                Module.ENV[key] = value;
            }
        }

        // Module.ENV["DOTNET_SYSTEM_GLOBALIZATION_INVARIANT"] = "true";
    };
    Module.preRun = [corePreRun];

    const runtimeApi = {
        Module,
        INTERNAL: {},
        runtimeId: 0,
        runtimeBuildInfo: {
            productVersion: "corerun",
            gitHash: "corerun",
            buildConfiguration: "corerun",
            wasmEnableThreads: false,
            wasmEnableSIMD: true,
            wasmEnableExceptionHandling: true,
        },
    };
    dotnetInternals = [
        runtimeApi,
        [],
    ];

    createDotnetRuntime(runtimeApi.Module);
}
selfRun();


Review comment (suggestion):
please revert this, coreRun should stay invariant. we don't want to replicate loader logic here


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

/* eslint-disable no-undef */
/* eslint-disable space-before-function-paren */
/* eslint-disable @typescript-eslint/no-unused-vars */
var fetch = fetch || undefined; var dotnetNativeModuleLoaded = false; var dotnetInternals = null;
export function selfRun() {
    const Module = {};
    const corePreRun = () => {
        // copy all node/shell env variables to emscripten env
        if (globalThis.process && globalThis.process.env) {
            for (const [key, value] of Object.entries(process.env)) {
                Module.ENV[key] = value;
            }
        }

        Module.ENV["DOTNET_SYSTEM_GLOBALIZATION_INVARIANT"] = "true";
    };
    Module.preRun = [corePreRun];

    const runtimeApi = {
        Module,
        INTERNAL: {},
        runtimeId: 0,
        runtimeBuildInfo: {
            productVersion: "corerun",
            gitHash: "corerun",
            buildConfiguration: "corerun",
            wasmEnableThreads: false,
            wasmEnableSIMD: true,
            wasmEnableExceptionHandling: true,
        },
    };
    dotnetInternals = [
        runtimeApi,
        [],
    ];

    createDotnetRuntime(runtimeApi.Module);
}
selfRun();