ronantakizawa/codereview-bench huggingface dataset:
Repository: dotnet/runtime
File: src/libraries/System.Net.Security/src/System/Net/Security/SslClientAuthenticationOptions.cs (C#)

Code before:
#pragma warning disable SYSLIB0040 // NoEncryption and AllowNoEncryption are obsolete
                if (value != EncryptionPolicy.RequireEncryption && value != EncryptionPolicy.AllowNoEncryption && value != EncryptionPolicy.NoEncryption)
                {
                    throw new ArgumentException(SR.Format(SR.net_invalid_enum, nameof(EncryptionPolicy)), nameof(value));
                }
#pragma warning restore SYSLIB0040

                _encryptionPolicy = value;
            }
        }

        public SslProtocols EnabledSslProtocols
        {
            get => _enabledSslProtocols;
            set => _enabledSslProtocols = value;
        }

        /// <summary>
        /// Specifies cipher suites allowed to be used for TLS.
        /// When set to null operating system default will be used.
        /// Use extreme caution when changing this setting.
        /// </summary>
        public CipherSuitesPolicy? CipherSuitesPolicy { get; set; }

        /// <summary>
        /// Specifies X509ChainPolicy to use for remote certificate
        /// validation. If set, CertificateRevocationCheckMode and SslCertificateTrust is ignored.
        /// </summary>
        public X509ChainPolicy? ValidationPolicy { get; set; }
    }
}


Review comment (suggestion):
Following the style from https://github.com/dotnet/dotnet-api-docs/wiki/Summary%3A-Property

```suggestion
        /// Gets or sets an optional customized policy for remote certificate
```

Code after:
#pragma warning disable SYSLIB0040 // NoEncryption and AllowNoEncryption are obsolete
                if (value != EncryptionPolicy.RequireEncryption && value != EncryptionPolicy.AllowNoEncryption && value != EncryptionPolicy.NoEncryption)
                {
                    throw new ArgumentException(SR.Format(SR.net_invalid_enum, nameof(EncryptionPolicy)), nameof(value));
                }
#pragma warning restore SYSLIB0040

                _encryptionPolicy = value;
            }
        }

        public SslProtocols EnabledSslProtocols
        {
            get => _enabledSslProtocols;
            set => _enabledSslProtocols = value;
        }

        /// <summary>
        /// Specifies cipher suites allowed to be used for TLS.
        /// When set to null operating system default will be used.
        /// Use extreme caution when changing this setting.
        /// </summary>
        public CipherSuitesPolicy? CipherSuitesPolicy { get; set; }

        /// <summary>
        /// Gets or sets an optional customized policy for remote certificate
        /// validation. If not <see langword="null"/>,
        /// <see cref="CertificateRevocationCheckMode"/> and <see cref="SslCertificateTrust"/>
        /// are ignored.
        /// </summary>
        public X509ChainPolicy? CertificateChainPolicy { get; set; }
    }
}
Repository: dotnet/runtime
File: src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509ChainPolicy.cs (C#)

Code before:
        public DateTime VerificationTime
        {
            get => _verificationTime;
            set
            {
                _verificationTime = value;
                VerificationTimeIgnored = false;
            }
        }

        public TimeSpan UrlRetrievalTimeout { get; set; }

        public void Reset()
        {
            _applicationPolicy = null;
            _certificatePolicy = null;
            _extraStore = null;
            _customTrustStore = null;
            DisableCertificateDownloads = false;
            _revocationMode = X509RevocationMode.Online;
            _revocationFlag = X509RevocationFlag.ExcludeRoot;
            _verificationFlags = X509VerificationFlags.NoFlag;
            _trustMode = X509ChainTrustMode.System;
            _verificationTime = DateTime.Now;
            VerificationTimeIgnored = true;

            UrlRetrievalTimeout = TimeSpan.Zero; // default timeout
        }

        public X509ChainPolicy Clone()
        {
            return (X509ChainPolicy)MemberwiseClone();
        }
    }
}


Review comment (suggestion):
This blank line doesn't seem to make sense in context.

```suggestion
```

Code after:

        public DateTime VerificationTime
        {
            get => _verificationTime;
            set
            {
                _verificationTime = value;
                VerificationTimeIgnored = false;
            }
        }

        public TimeSpan UrlRetrievalTimeout { get; set; }

        public void Reset()
        {
            _applicationPolicy = null;
            _certificatePolicy = null;
            _extraStore = null;
            _customTrustStore = null;
            DisableCertificateDownloads = false;
            _revocationMode = X509RevocationMode.Online;
            _revocationFlag = X509RevocationFlag.ExcludeRoot;
            _verificationFlags = X509VerificationFlags.NoFlag;
            _trustMode = X509ChainTrustMode.System;
            _verificationTime = DateTime.Now;
            VerificationTimeIgnored = true;
            UrlRetrievalTimeout = TimeSpan.Zero; // default timeout
        }

        public X509ChainPolicy Clone()
        {
            X509ChainPolicy clone = new X509ChainPolicy
            {
                DisableCertificateDownloads = DisableCertificateDownloads,
                _revocationMode = _revocationMode,
                _revocationFlag = _revocationFlag,
                _verificationFlags = _verificationFlags,
                _trustMode = _trustMode,
                _verificationTime = _verificationTime,
                VerificationTimeIgnored = VerificationTimeIgnored,
                UrlRetrievalTimeout = UrlRetrievalTimeout,
            };

            if (_applicationPolicy?.Count > 0)
            {
                foreach (var item in _applicationPolicy)
                {
                    clone.ApplicationPolicy.Add(item);
                }
            }
Repository: dotnet/runtime
File: src/coreclr/pal/src/thread/thread.cpp (C++)

Code before:
#include <unistd.h>
#include <errno.h>
#include <stddef.h>
#include <sys/stat.h>
#include <sys/mman.h>
#if HAVE_MACH_THREADS
#include <mach/mach.h>
#endif // HAVE_MACH_THREADS
#if HAVE_POLL
#include <poll.h>
#else
#include "pal/fakepoll.h"
#endif  // HAVE_POLL
#include <limits.h>
#include <algorithm>

#if HAVE_SYS_LWP_H
#include <sys/lwp.h>
#endif
#if HAVE_LWP_H
#include <lwp.h>
#endif
// If we don't have sys/lwp.h but do expect to use _lwp_self, declare it to silence compiler warnings
#if HAVE__LWP_SELF && !defined(HAVE_SYS_LWP_H) && !defined(HAVE_LWP_H)
extern "C" int _lwp_self ();
#endif

#if HAVE_CPUSET_T
typedef cpuset_t cpu_set_t;
#endif

using namespace CorUnix;

#ifdef __APPLE__
#define MAX_THREAD_NAME_SIZE 63
#elif defined(__FreeBSD__)
#define MAX_THREAD_NAME_SIZE MAXCOMLEN
#else
#define MAX_THREAD_NAME_SIZE 15
#endif

/* ------------------- Definitions ------------------------------*/

// Activation function that gets called when an activation is injected into a thread.
PAL_ActivationFunction g_activationFunction = NULL;
// Function to check if an activation can be safely injected at a specified context
PAL_SafeActivationCheckFunction g_safeActivationCheckFunction = NULL;

void
ThreadCleanupRoutine(
    CPalThread *pThread,

Review comment (refactor):
Note: `HAVE_SYS_LWP_H` (and  maybe `HAVE_LWP_H`) are defined here, but as zero.
Now that `_KERNEL` is not defined in here, the _lwp_self() declaration is visible,
so we could probably just get rid of the three lines above.

If we leave this exposed now, it causes duplicate/conflicting declaration errors.


Code after:
#include <unistd.h>
#include <errno.h>
#include <stddef.h>
#include <sys/stat.h>
#include <sys/mman.h>
#if HAVE_MACH_THREADS
#include <mach/mach.h>
#endif // HAVE_MACH_THREADS
#if HAVE_POLL
#include <poll.h>
#else
#include "pal/fakepoll.h"
#endif  // HAVE_POLL
#include <limits.h>
#include <algorithm>

#if HAVE_SYS_LWP_H
#include <sys/lwp.h>
#endif
#if HAVE_LWP_H
#include <lwp.h>
#endif

#if HAVE_CPUSET_T
typedef cpuset_t cpu_set_t;
#endif

using namespace CorUnix;

#ifdef __APPLE__
#define MAX_THREAD_NAME_SIZE 63
#elif defined(__FreeBSD__)
#define MAX_THREAD_NAME_SIZE MAXCOMLEN
#else
#define MAX_THREAD_NAME_SIZE 15
#endif

/* ------------------- Definitions ------------------------------*/

// Activation function that gets called when an activation is injected into a thread.
PAL_ActivationFunction g_activationFunction = NULL;
// Function to check if an activation can be safely injected at a specified context
PAL_SafeActivationCheckFunction g_safeActivationCheckFunction = NULL;

void
ThreadCleanupRoutine(
    CPalThread *pThread,
    IPalObject *pObjectToCleanup,
    bool fShutdown,
    bool fCleanupSharedState
    );
Repository: dotnet/runtime
File: src/coreclr/pal/src/thread/thread.cpp (C++)

Code before:
#include <unistd.h>
#include <errno.h>
#include <stddef.h>
#include <sys/stat.h>
#include <sys/mman.h>
#if HAVE_MACH_THREADS
#include <mach/mach.h>
#endif // HAVE_MACH_THREADS
#if HAVE_POLL
#include <poll.h>
#else
#include "pal/fakepoll.h"
#endif  // HAVE_POLL
#include <limits.h>
#include <algorithm>

#if HAVE_SYS_LWP_H
#include <sys/lwp.h>
#endif
#if HAVE_LWP_H
#include <lwp.h>
#endif
// If we don't have sys/lwp.h but do expect to use _lwp_self, declare it to silence compiler warnings
#if HAVE__LWP_SELF && !HAVE_SYS_LWP_H && !HAVE_LWP_H
extern "C" int _lwp_self ();
#endif

#if HAVE_CPUSET_T
typedef cpuset_t cpu_set_t;
#endif

using namespace CorUnix;

#ifdef __APPLE__
#define MAX_THREAD_NAME_SIZE 63
#elif defined(__FreeBSD__)
#define MAX_THREAD_NAME_SIZE MAXCOMLEN
#else
#define MAX_THREAD_NAME_SIZE 15
#endif

/* ------------------- Definitions ------------------------------*/

// Activation function that gets called when an activation is injected into a thread.
PAL_ActivationFunction g_activationFunction = NULL;
// Function to check if an activation can be safely injected at a specified context
PAL_SafeActivationCheckFunction g_safeActivationCheckFunction = NULL;

void
ThreadCleanupRoutine(
    CPalThread *pThread,

Review comment (suggestion):
The local declaration was only needed because this used to define `_KERNEL` on sunos, which masks the declaration sunos provides in `sys/lwp.h`.  With that fixed, this bit can just go away.

Code after:
#if HAVE_MACH_THREADS
#include <mach/mach.h>
#endif // HAVE_MACH_THREADS
#if HAVE_POLL
#include <poll.h>
#else
#include "pal/fakepoll.h"
#endif  // HAVE_POLL
#include <limits.h>
#include <algorithm>

#if HAVE_SYS_LWP_H
#include <sys/lwp.h>
#endif
#if HAVE_LWP_H
#include <lwp.h>
#endif

#if HAVE_CPUSET_T
typedef cpuset_t cpu_set_t;
#endif

using namespace CorUnix;

#ifdef __APPLE__
#define MAX_THREAD_NAME_SIZE 63
#elif defined(__FreeBSD__)
#define MAX_THREAD_NAME_SIZE MAXCOMLEN
#else
#define MAX_THREAD_NAME_SIZE 15
#endif

/* ------------------- Definitions ------------------------------*/

// Activation function that gets called when an activation is injected into a thread.
PAL_ActivationFunction g_activationFunction = NULL;
// Function to check if an activation can be safely injected at a specified context
PAL_SafeActivationCheckFunction g_safeActivationCheckFunction = NULL;

void
ThreadCleanupRoutine(
    CPalThread *pThread,
    IPalObject *pObjectToCleanup,
    bool fShutdown,
    bool fCleanupSharedState
    );

PAL_ERROR
ThreadInitializationRoutine(
    CPalThread *pThread,
    CObjectType *pObjectType,
Repository: dotnet/runtime
File: src/coreclr/jit/simd.h (C/C++)

Code before:

        default:
        {
            unreached();
        }
    }
}

template <typename TSimd, typename TBase>
void NarrowSimdLong(TSimd* result, const TSimd& arg0)
{
    uint32_t count = sizeof(TSimd) / sizeof(TBase);

    for (uint32_t i = 0; i < count; i++)
    {
        uint64_t input0;
        memcpy(&input0, &arg0.u8[(i * sizeof(TBase) / sizeof(uint64_t)) * sizeof(uint64_t)], sizeof(uint64_t));

        // Saturate to largest value for TBase
        if (input0 > (TBase)-1)
        {
            input0 = (TBase)-1;
        }

        memcpy(&result->u8[i * sizeof(TBase)], &input0, sizeof(TBase));
    }
}

template <typename TSimd>
void NarrowSimdLong(var_types baseType, TSimd* result, TSimd arg0)
{
    switch (baseType)
    {
        case TYP_FLOAT:
        case TYP_INT:
        case TYP_UINT:
        {
            NarrowSimdLong<TSimd, uint32_t>(result, arg0);
            break;
        }

        case TYP_DOUBLE:
        case TYP_LONG:
        case TYP_ULONG:
        {
            NarrowSimdLong<TSimd, uint64_t>(result, arg0);
            break;
        }

        case TYP_BYTE:
        case TYP_UBYTE:

Review comment (bug):
The loop count calculation appears incorrect. The function is meant to narrow uint64_t values to TBase type, but the loop iterates `sizeof(TSimd) / sizeof(TBase)` times. 

For a 16-byte SIMD with TBase=uint8_t:
- Loop runs 16 iterations
- But there are only 2 uint64_t values (16 bytes / 8 bytes)
- The function should iterate `sizeof(TSimd) / sizeof(uint64_t)` times instead

This causes the loop to read beyond the available uint64_t values and write narrowed results to incorrect positions.

Code after:

        default:
        {
            unreached();
        }
    }
}

template <typename TSimd, typename TBase>
void NarrowSimdLong(TSimd* result, const TSimd& arg0)
{
    uint32_t count = sizeof(TSimd) / sizeof(uint64_t);

    for (uint32_t i = 0; i < count; i++)
    {
        uint64_t input0;
        memcpy(&input0, &arg0.u8[(i * sizeof(TBase) / sizeof(uint64_t)) * sizeof(uint64_t)], sizeof(uint64_t));

        // Saturate to largest value for TBase
        if (input0 > (TBase)-1)
        {
            input0 = (TBase)-1;
        }

        memcpy(&result->u8[i * sizeof(TBase)], &input0, sizeof(TBase));
    }
}

template <typename TSimd>
void NarrowSimdLong(var_types baseType, TSimd* result, const TSimd& arg0)
{
    switch (baseType)
    {
        case TYP_FLOAT:
        case TYP_INT:
        case TYP_UINT:
        {
            NarrowSimdLong<TSimd, uint32_t>(result, arg0);
            break;
        }

        case TYP_DOUBLE:
        case TYP_LONG:
        case TYP_ULONG:
        {
            NarrowSimdLong<TSimd, uint64_t>(result, arg0);
            break;
        }

        case TYP_BYTE:
        case TYP_UBYTE:
Repository: dotnet/runtime
File: src/coreclr/jit/simd.h (C/C++)

Code before:
        }
    }
}

template <typename TSimd, typename TBase>
void NarrowSimdLong(TSimd* result, const TSimd& arg0)
{
    uint32_t count = sizeof(TSimd) / sizeof(TBase);

    for (uint32_t i = 0; i < count; i++)
    {
        uint64_t input0;
        memcpy(&input0, &arg0.u8[(i * sizeof(TBase) / sizeof(uint64_t)) * sizeof(uint64_t)], sizeof(uint64_t));

        // Saturate to largest value for TBase
        if (input0 > (TBase)-1)
        {
            input0 = (TBase)-1;
        }

        memcpy(&result->u8[i * sizeof(TBase)], &input0, sizeof(TBase));
    }
}

template <typename TSimd>
void NarrowSimdLong(var_types baseType, TSimd* result, TSimd arg0)
{
    switch (baseType)
    {
        case TYP_FLOAT:
        case TYP_INT:
        case TYP_UINT:
        {
            NarrowSimdLong<TSimd, uint32_t>(result, arg0);
            break;
        }

        case TYP_DOUBLE:
        case TYP_LONG:
        case TYP_ULONG:
        {
            NarrowSimdLong<TSimd, uint64_t>(result, arg0);
            break;
        }

        case TYP_BYTE:
        case TYP_UBYTE:
        {
            NarrowSimdLong<TSimd, uint8_t>(result, arg0);
            break;
        }

Review comment (performance):
The `arg0` parameter is passed by value, creating an unnecessary copy of a 16-byte structure. Consider changing to `const TSimd& arg0` for consistency with the inner template function and better performance.
```suggestion
void NarrowSimdLong(var_types baseType, TSimd* result, const TSimd& arg0)
```

Code after:
        }
    }
}

template <typename TSimd, typename TBase>
void NarrowSimdLong(TSimd* result, const TSimd& arg0)
{
    uint32_t count = sizeof(TSimd) / sizeof(uint64_t);

    for (uint32_t i = 0; i < count; i++)
    {
        uint64_t input0;
        memcpy(&input0, &arg0.u8[(i * sizeof(TBase) / sizeof(uint64_t)) * sizeof(uint64_t)], sizeof(uint64_t));

        // Saturate to largest value for TBase
        if (input0 > (TBase)-1)
        {
            input0 = (TBase)-1;
        }

        memcpy(&result->u8[i * sizeof(TBase)], &input0, sizeof(TBase));
    }
}

template <typename TSimd>
void NarrowSimdLong(var_types baseType, TSimd* result, const TSimd& arg0)
{
    switch (baseType)
    {
        case TYP_FLOAT:
        case TYP_INT:
        case TYP_UINT:
        {
            NarrowSimdLong<TSimd, uint32_t>(result, arg0);
            break;
        }

        case TYP_DOUBLE:
        case TYP_LONG:
        case TYP_ULONG:
        {
            NarrowSimdLong<TSimd, uint64_t>(result, arg0);
            break;
        }

        case TYP_BYTE:
        case TYP_UBYTE:
        {
            NarrowSimdLong<TSimd, uint8_t>(result, arg0);
            break;
        }
Repository: dotnet/runtime
File: src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs (C#)

Code before:
#if TARGET_64BIT
            // See comment in Span<T>.Slice for how this works.
            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 ReadOnlySpan<T>(ref Unsafe.Add(ref _reference, (nint)(uint)start /* force zero-extension */), length);
        }

        /// <summary>
        /// Copies the contents of this read-only 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>
        public T[] ToArray()
        {
            if (IsEmpty)
            {
                return Array.Empty<T>();
            }

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


Review comment (suggestion):
nit: Only this line needs to be changed to reduce unsafe.

`destination` is arguably a clearer name and the use of `_length == 0` and `new T[_length]` isn't at all unsafe, plus it reduces inlining needs for a known perf critical type so we only have one path that actually needs inlining considerations.

Code after:
#if TARGET_64BIT
            // See comment in Span<T>.Slice for how this works.
            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 ReadOnlySpan<T>(ref Unsafe.Add(ref _reference, (nint)(uint)start /* force zero-extension */), length);
        }

        /// <summary>
        /// Copies the contents of this read-only 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>
        public T[] ToArray()
        {
            if (IsEmpty)
            {
                return Array.Empty<T>();
            }

            var destination = new T[Length];
            CopyTo(destination);
            return destination;
        }
    }
}
Repository: dotnet/runtime
File: src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs (C#)

Code before:
            // See comment in Span<T>.Slice for how this works.
            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 ReadOnlySpan<T>(ref Unsafe.Add(ref _reference, (nint)(uint)start /* force zero-extension */), length);
        }

        /// <summary>
        /// Copies the contents of this read-only 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>
        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:
            // See comment in Span<T>.Slice for how this works.
            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 ReadOnlySpan<T>(ref Unsafe.Add(ref _reference, (nint)(uint)start /* force zero-extension */), length);
        }

        /// <summary>
        /// Copies the contents of this read-only 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>
        public T[] ToArray()
        {
            if (IsEmpty)
            {
                return Array.Empty<T>();
            }

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