ronantakizawa/codereview-bench huggingface dataset:
Repository: dotnet/runtime
File: src/libraries/System.Private.CoreLib/src/System/Threading/TimerQueue.Windows.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.Diagnostics;

namespace System.Threading
{
    internal sealed partial class TimerQueue
    {
        private TimerQueue(int id)
        {
            _id = id;
        }

        public static long TickCount64 => Environment.TickCount64;

        private bool SetTimer(uint actualDuration) =>
            ThreadPool.UseWindowsThreadPool ?
            SetTimerWindowsThreadPool(actualDuration) :
            SetTimerPortable(actualDuration);
    }
}


Review comment (suggestion):
I thought about preserving, but inversing this comment (calling out the potential desync that can occur on Win7). However, we don't support OS prior to Win8/Server 2012 anymore and so such considerations are now largely irrelevant outside of just historical context.

We're now simply matching the behavior to not include "bias" (sleep/hibernation) time that is used on all supported OS and so directly using `Environment.TickCount64` is always fine for us.

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.Diagnostics;

namespace System.Threading
{
    internal sealed partial class TimerQueue
    {
        private TimerQueue(int id)
        {
            _id = id;
        }

        private bool SetTimer(uint actualDuration) =>
            ThreadPool.UseWindowsThreadPool ?
            SetTimerWindowsThreadPool(actualDuration) :
            SetTimerPortable(actualDuration);
    }
}
Repository: dotnet/runtime
File: src/libraries/Common/src/Interop/Windows/Kernel32/Interop.QueryUnbiasedInterruptTime.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;

internal static partial class Interop
{
    internal static partial class Kernel32
    {
        // The actual native signature is:
        //      BOOL WINAPI QueryUnbiasedInterruptTime(
        //          _Out_ PULONGLONG UnbiasedTime
        //      );
        //
        // We take a long* (rather than a out long) to avoid the pinning overhead.
        // We don't set last error since we don't need the extended error info.

        [LibraryImport(Libraries.Kernel32)]
        [SuppressGCTransition]
        internal static unsafe partial BOOL QueryUnbiasedInterruptTime(ulong* unbiasedTime);
    }
}


Review comment (suggestion):
The comment mentions "long*" but the actual parameter type is "ulong*". The comment should be updated to accurately reflect that we're taking a ulong* parameter to match the native ULONGLONG type.
```suggestion
        // We take a ulong* (rather than a out ulong) to avoid the pinning overhead.
```

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.Runtime.InteropServices;

internal static partial class Interop
{
    internal static partial class Kernel32
    {
        // The actual native signature is:
        //      BOOL WINAPI QueryUnbiasedInterruptTime(
        //          _Out_ PULONGLONG UnbiasedTime
        //      );
        //
        // We take a ulong* (rather than a out ulong) to avoid the pinning overhead.
        // We don't set last error since we don't need the extended error info.

        [LibraryImport(Libraries.Kernel32)]
        [SuppressGCTransition]
        internal static unsafe partial BOOL QueryUnbiasedInterruptTime(ulong* unbiasedTime);
    }
}
Repository: dotnet/runtime
File: src/native/libs/System.Security.Cryptography.Native/openssl.c (C)

Code before:
    if (x509)
    {
        return (int32_t)X509_get_version(x509);
    }

    return -1;
}

/*
Function:
GetX509PublicKeyAlgorithm

Used by System.Security.Cryptography.X509Certificates' OpenSslX509CertificateReader to identify the
algorithm the public key is associated with.

Return values:
NULL if the algorithm cannot be determined, otherwise a pointer to the OpenSSL ASN1_OBJECT structure
describing the object type.
*/
ASN1_OBJECT* CryptoNative_GetX509PublicKeyAlgorithm(X509* x509)
{
    // No error queue impact, all of the called routines are just field accessors.

    if (x509)
    {
        const X509_PUBKEY* pubkey = X509_get_X509_PUBKEY(x509);
        ASN1_OBJECT* algOid;

        if (pubkey && X509_PUBKEY_get0_param(&algOid, NULL, NULL, NULL, pubkey))
        {
            return algOid;
        }
    }

    return NULL;
}

/*
Function:
GetX509SignatureAlgorithm

Used by System.Security.Cryptography.X509Certificates' OpenSslX509CertificateReader to identify the
algorithm used by the Certificate Authority for signing the certificate.

Return values:
NULL if the algorithm cannot be determined, otherwise a pointer to the OpenSSL ASN1_OBJECT structure
describing the object type.
*/
ASN1_OBJECT* CryptoNative_GetX509SignatureAlgorithm(X509* x509)
{
    // No error queue impact.

Review comment (suggestion):
~This is only used by `X509_PUBKEY_get0_param` below, which already has the parameter qualified as `const`, so we can just mark our variable as const and it will still work for the call.~

Okay, that is not true with OpenSSL 1.1.x. We are going to have to discard the qualifiers.

Code after:
    if (x509)
    {
        return (int32_t)X509_get_version(x509);
    }

    return -1;
}

/*
Function:
GetX509PublicKeyAlgorithm

Used by System.Security.Cryptography.X509Certificates' OpenSslX509CertificateReader to identify the
algorithm the public key is associated with.

Return values:
NULL if the algorithm cannot be determined, otherwise a pointer to the OpenSSL ASN1_OBJECT structure
describing the object type.
*/
ASN1_OBJECT* CryptoNative_GetX509PublicKeyAlgorithm(X509* x509)
{
    // No error queue impact, all of the called routines are just field accessors.

    if (x509)
    {
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wcast-qual"
        // In OpenSSL 4.0 this is const X509_PUBKEY*. OpenSSL 1.1.1's X509_PUBKEY_get0_param
        // does not accept a const X509_PUBKEY*.
        X509_PUBKEY* pubkey = (X509_PUBKEY*)X509_get_X509_PUBKEY(x509);
#pragma clang diagnostic pop
        ASN1_OBJECT* algOid;

        if (pubkey && X509_PUBKEY_get0_param(&algOid, NULL, NULL, NULL, pubkey))
        {
            return algOid;
        }
    }

    return NULL;
}

/*
Function:
GetX509SignatureAlgorithm

Used by System.Security.Cryptography.X509Certificates' OpenSslX509CertificateReader to identify the
algorithm used by the Certificate Authority for signing the certificate.

Return values:
NULL if the algorithm cannot be determined, otherwise a pointer to the OpenSSL ASN1_OBJECT structure
Repository: dotnet/runtime
File: src/native/libs/System.Security.Cryptography.Native/openssl.c (C)

Code before:

    return 0;
}

/*
Function:
GetX509PublicKeyBytes

Used by System.Security.Cryptography.X509Certificates' OpenSslX509CertificateReader to obtain the
raw bytes of the public key.

Return values:
NULL if the public key cannot be determined, a pointer to the ASN1_BIT_STRING structure representing
the public key.
*/
const ASN1_BIT_STRING* CryptoNative_GetX509PublicKeyBytes(X509* x509)
{
    // No error queue impact.

    if (x509)
    {
        return X509_get0_pubkey_bitstr(x509);
    }

    return NULL;
}

/*
Function:
GetAsn1StringBytes

Used by the NativeCrypto shim type to extract byte[] data from OpenSSL ASN1_* types whenever a byte[] is called
for in managed code.

Return values:
0: Invalid X509 pointer
1: Data was copied
Any negative value: The input buffer size was reported as insufficient. A buffer of size ABS(return) is required.

Remarks:
 Many ASN1 types are actually the same type in OpenSSL:
   STRING
   INTEGER
   ENUMERATED
   BIT_STRING
   OCTET_STRING
   PRINTABLESTRING
   T61STRING
   IA5STRING
   GENERALSTRING
   UNIVERSALSTRING

Review comment (suggestion):
This interior pointer is later fed in to `CryptoNative_GetAsn1StringBytes` which does not modify the structure. So while the const is "erased" across the p/invoke boundary, the uses of the interior pointer do not violate the const requirement.

Code after:
        return -len;
    }

    unsigned char* pBuf2 = pBuf;
    len = i2d_ASN1_TYPE(parameter, &pBuf2);

    if (len > 0)
    {
        return 1;
    }

    return 0;
}

/*
Function:
GetX509PublicKeyBytes

Used by System.Security.Cryptography.X509Certificates' OpenSslX509CertificateReader to obtain the
raw bytes of the public key.

Return values:
NULL if the public key cannot be determined, a pointer to the ASN1_BIT_STRING structure representing
the public key.
*/
const ASN1_BIT_STRING* CryptoNative_GetX509PublicKeyBytes(X509* x509)
{
    // No error queue impact.

    if (x509)
    {
        return X509_get0_pubkey_bitstr(x509);
    }

    return NULL;
}

/*
Function:
GetAsn1StringBytes

Used by the NativeCrypto shim type to extract byte[] data from OpenSSL ASN1_* types whenever a byte[] is called
for in managed code.

Return values:
0: Invalid X509 pointer
1: Data was copied
Any negative value: The input buffer size was reported as insufficient. A buffer of size ABS(return) is required.

Remarks:
 Many ASN1 types are actually the same type in OpenSSL:
Repository: dotnet/runtime
File: src/mono/mono/mini/aot-runtime.c (C)

Code before:
	}

	/* See exception_cb () in mini-llvm.c as to why this is needed */
	nindex = ei_len;
	for (guint32 i = 0; i < ei_len; ++i) {
		gint32 cindex1 = read32 (type_info [i]);

		for (GSList *l = nesting [cindex1]; l; l = l->next) {
			gint32 nesting_cindex = GPOINTER_TO_INT (l->data);
			MonoJitExceptionInfo *nesting_ei;
			MonoJitExceptionInfo *nesting_clause = &clauses [nesting_cindex];

			nesting_ei = &jinfo->clauses [nindex];
			nindex ++;

			memcpy (nesting_ei, &jinfo->clauses [i], sizeof (MonoJitExceptionInfo));
			nesting_ei->flags = nesting_clause->flags;
			nesting_ei->data.catch_class = nesting_clause->data.catch_class;
			nesting_ei->clause_index = nesting_cindex;
		}
	}
	g_assert (nindex == ei_len + nested_len);
	if (!async) {
		g_free (ei);
		g_free (type_info);
		g_free (unw_info);
	}
}

static gpointer
alloc0_jit_info_data (MonoMemoryManager *mem_manager, int size, gboolean async_context)

#define alloc0_jit_info_data(mem_manager, size, async_context) (g_cast (alloc0_jit_info_data ((mem_manager), (size), (async_context))))

{
	gpointer res;

	if (async_context) {
		res = mono_mem_manager_alloc0_lock_free (mem_manager, size);
		mono_atomic_fetch_add_i32 (&async_jit_info_size, size);
	} else {
		res = mono_mem_manager_alloc0 (mem_manager, size);
	}
	return res;
}

/*
 * In async context, this is async safe.
 */
static MonoJitInfo*
decode_exception_debug_info (MonoAotModule *amodule,

Review comment (suggestion):
The value is stored in `jinfo_unwind->unw_info` unconditionally. Comments explicitly states that the allocation is leaked.

Code after:
	}

	/* See exception_cb () in mini-llvm.c as to why this is needed */
	nindex = ei_len;
	for (guint32 i = 0; i < ei_len; ++i) {
		gint32 cindex1 = read32 (type_info [i]);

		for (GSList *l = nesting [cindex1]; l; l = l->next) {
			gint32 nesting_cindex = GPOINTER_TO_INT (l->data);
			MonoJitExceptionInfo *nesting_ei;
			MonoJitExceptionInfo *nesting_clause = &clauses [nesting_cindex];

			nesting_ei = &jinfo->clauses [nindex];
			nindex ++;

			memcpy (nesting_ei, &jinfo->clauses [i], sizeof (MonoJitExceptionInfo));
			nesting_ei->flags = nesting_clause->flags;
			nesting_ei->data.catch_class = nesting_clause->data.catch_class;
			nesting_ei->clause_index = nesting_cindex;
		}
	}
	g_assert (nindex == ei_len + nested_len);
	if (!async) {
		g_free (ei);
		g_free (type_info);
	}
}

static gpointer
alloc0_jit_info_data (MonoMemoryManager *mem_manager, int size, gboolean async_context)

#define alloc0_jit_info_data(mem_manager, size, async_context) (g_cast (alloc0_jit_info_data ((mem_manager), (size), (async_context))))

{
	gpointer res;

	if (async_context) {
		res = mono_mem_manager_alloc0_lock_free (mem_manager, size);
		mono_atomic_fetch_add_i32 (&async_jit_info_size, size);
	} else {
		res = mono_mem_manager_alloc0 (mem_manager, size);
	}
	return res;
}

/*
 * In async context, this is async safe.
 */
static MonoJitInfo*
decode_exception_debug_info (MonoAotModule *amodule,
							 MonoMethod *method, guint8* ex_info,
Repository: dotnet/runtime
File: src/coreclr/nativeaot/Runtime/allocheap.h (C/C++)

Code before:
    bool _AllocNewBlock(uintptr_t cbMem);
    uint8_t* _AllocFromCurBlock(uintptr_t cbMem, uintptr_t alignment);
    bool _CommitFromCurBlock(uintptr_t cbMem);

    // Access protection helpers
    bool _UpdateMemPtrs(uint8_t* pNextFree, uint8_t* pFreeCommitEnd, uint8_t* pFreeReserveEnd);
    bool _UpdateMemPtrs(uint8_t* pNextFree, uint8_t* pFreeCommitEnd);
    bool _UpdateMemPtrs(uint8_t* pNextFree);

    static const uintptr_t BLOCK_SIZE = 4096;

    struct BlockListElem
    {
        BlockListElem* m_pNext;
        uintptr_t m_cbMem;

        uint8_t* GetStart() const { return (uint8_t*)this; }
        uintptr_t GetLength() const { return m_cbMem; }
        uint8_t* GetDataStart() const { return (uint8_t*)this + sizeof(BlockListElem); }
    };

    typedef SList<BlockListElem>    BlockList;
    BlockList                       m_blockList;

    uint8_t *                         m_pNextFree;
    uint8_t *                         m_pFreeCommitEnd;
    uint8_t *                         m_pFreeReserveEnd;

    Crst                            m_lock;

    INDEBUG(bool                    m_fIsInit;)
};
typedef DPTR(AllocHeap) PTR_AllocHeap;

//-------------------------------------------------------------------------------------------------
void * __cdecl operator new(size_t n, AllocHeap * alloc);
void * __cdecl operator new[](size_t n, AllocHeap * alloc);



Review comment (suggestion):
Delete `m_pNextFree`, `m_pFreeCommitEnd`, `m_pFreeReserveEnd` and replace it with a simple state that just tracks amount of memory used in the current block. Delete `_UpdateMemPtrs` method as part of this simplification

Code after:
    uint8_t* _AllocFromCurBlock(uintptr_t cbMem, uintptr_t alignment);

    static const uintptr_t BLOCK_SIZE = 4096;

    struct BlockListElem
    {
        BlockListElem* m_pNext;
        uintptr_t m_cbMem;

        uintptr_t GetLength() const { return m_cbMem; }
        uint8_t* GetDataStart() const { return (uint8_t*)this + sizeof(BlockListElem); }
    };

    typedef SList<BlockListElem>    BlockList;
    BlockList                       m_blockList;

    uintptr_t                       m_cbCurBlockUsed;

    CrstStatic                      m_lock;

    INDEBUG(bool                    m_fIsInit;)
};
typedef DPTR(AllocHeap) PTR_AllocHeap;

//-------------------------------------------------------------------------------------------------
void * __cdecl operator new(size_t n, AllocHeap * alloc);
void * __cdecl operator new[](size_t n, AllocHeap * alloc);
Repository: dotnet/runtime
File: src/coreclr/nativeaot/Runtime/allocheap.cpp (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.
#include "common.h"
#include "CommonTypes.h"
#include "CommonMacros.h"
#include "daccess.h"
#include "DebugMacrosExt.h"
#include "PalLimitedContext.h"
#include "Pal.h"
#include "rhassert.h"
#include "slist.h"
#include "holder.h"
#include "Crst.h"
#include "allocheap.h"

#include "CommonMacros.inl"
#include "slist.inl"

//-------------------------------------------------------------------------------------------------
AllocHeap::AllocHeap()
    : m_blockList(),
      m_pCurBlock(NULL),
      m_cbCurBlockUsed(0),
      m_lock(CrstAllocHeap)
      COMMA_INDEBUG(m_fIsInit(false))
{
}

//-------------------------------------------------------------------------------------------------
bool AllocHeap::Init()
{
    ASSERT(!m_fIsInit);
    INDEBUG(m_fIsInit = true;)

    return true;
}

//-------------------------------------------------------------------------------------------------
AllocHeap::~AllocHeap()
{
    while (!m_blockList.IsEmpty())
    {
        BlockListElem *pCur = m_blockList.PopHead();
        delete[] (uint8_t*)pCur;
    }
}


Review comment (suggestion):
@copilot Delete `m_pCurBlock` and replace its uses with `m_blockList.GetHead()`

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
#include "common.h"
#include "CommonTypes.h"
#include "CommonMacros.h"
#include "daccess.h"
#include "DebugMacrosExt.h"
#include "PalLimitedContext.h"
#include "Pal.h"
#include "rhassert.h"
#include "slist.h"
#include "holder.h"
#include "Crst.h"
#include "allocheap.h"

#include "CommonMacros.inl"
#include "slist.inl"

//-------------------------------------------------------------------------------------------------
AllocHeap::AllocHeap()
    : m_blockList(),
      m_cbCurBlockUsed(0)
      COMMA_INDEBUG(m_fIsInit(false))
{
}

//-------------------------------------------------------------------------------------------------
bool AllocHeap::Init()
{
    ASSERT(!m_fIsInit);
    m_lock.Init(CrstAllocHeap);
    INDEBUG(m_fIsInit = true;)

    return true;
}

//-------------------------------------------------------------------------------------------------
void AllocHeap::Destroy()
{
    while (!m_blockList.IsEmpty())
    {
        BlockListElem *pCur = m_blockList.PopHead();
        delete[] (uint8_t*)pCur;
    }
    m_lock.Destroy();
}
Repository: dotnet/runtime
File: src/coreclr/nativeaot/Runtime/allocheap.h (C/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.
#include "forward_declarations.h"

class AllocHeap
{
  public:
    AllocHeap();

    bool Init();

    ~AllocHeap();

    // If AllocHeap was created with a MemAccessMgr, pRWAccessHolder must be non-NULL.
    // On return, the holder will permit R/W access to the allocated memory until it
    // is destructed.
    uint8_t * Alloc(uintptr_t cbMem);

    // If AllocHeap was created with a MemAccessMgr, pRWAccessHolder must be non-NULL.
    // On return, the holder will permit R/W access to the allocated memory until it
    // is destructed.
    uint8_t * AllocAligned(uintptr_t cbMem,
                         uintptr_t alignment);

  private:
    // Allocation Helpers
    uint8_t* _Alloc(uintptr_t cbMem, uintptr_t alignment);
    bool _AllocNewBlock(uintptr_t cbMem);
    uint8_t* _AllocFromCurBlock(uintptr_t cbMem, uintptr_t alignment);

    static const uintptr_t BLOCK_SIZE = 4096;

    struct BlockListElem
    {
        BlockListElem* m_pNext;
        uintptr_t m_cbMem;


Review comment (suggestion):
@copilot change `g_pAllocHeap` to `g_allocHeap` to avoid dynamic allocation. As part of that: change `AllocHeap::~AllocHeap` to Destroy method, , change the type of `AllocHeap::m_lock` to CrstStatic

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

class AllocHeap
{
  public:
    AllocHeap();

    bool Init();

    void Destroy();

    // If AllocHeap was created with a MemAccessMgr, pRWAccessHolder must be non-NULL.
    // On return, the holder will permit R/W access to the allocated memory until it
    // is destructed.
    uint8_t * Alloc(uintptr_t cbMem);

    // If AllocHeap was created with a MemAccessMgr, pRWAccessHolder must be non-NULL.
    // On return, the holder will permit R/W access to the allocated memory until it
    // is destructed.
    uint8_t * AllocAligned(uintptr_t cbMem,
                         uintptr_t alignment);

  private:
    // Allocation Helpers
    bool _AllocNewBlock(uintptr_t cbMem, uintptr_t alignment);
    uint8_t* _AllocFromCurBlock(uintptr_t cbMem, uintptr_t alignment);

    static const uintptr_t BLOCK_SIZE = 4096;

    struct BlockListElem
    {
        BlockListElem* m_pNext;
        uintptr_t m_cbMem;

        uintptr_t GetLength() const { return m_cbMem; }
Repository: dotnet/runtime
File: src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.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.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Text.Json.Nodes;
using System.Text.Json.Serialization.Converters;
using System.Text.Json.Serialization.Metadata;

namespace System.Text.Json.Serialization
{
    /// <summary>
    /// Converts an object or value to or from JSON.
    /// </summary>
    /// <typeparam name="T">The <see cref="System.Type"/> to convert.</typeparam>
    public abstract partial class JsonConverter<T> : JsonConverter
    {
        /// <summary>
        /// When overridden, constructs a new <see cref="JsonConverter{T}"/> instance.
        /// </summary>
        protected internal JsonConverter()
        {
            IsValueType = typeof(T).IsValueType;

            if (HandleNull)
            {
                HandleNullOnRead = true;
                HandleNullOnWrite = true;
            }
            else if (UsesDefaultHandleNull)
            {

Review comment (bug):
@copilot please remove this using by adding a qualification in the the debug assert in line 463.

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.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Text.Json.Serialization.Converters;
using System.Text.Json.Serialization.Metadata;

namespace System.Text.Json.Serialization
{
    /// <summary>
    /// Converts an object or value to or from JSON.
    /// </summary>
    /// <typeparam name="T">The <see cref="System.Type"/> to convert.</typeparam>
    public abstract partial class JsonConverter<T> : JsonConverter
    {
        /// <summary>
        /// When overridden, constructs a new <see cref="JsonConverter{T}"/> instance.
        /// </summary>
        protected internal JsonConverter()
        {
            IsValueType = typeof(T).IsValueType;

            if (HandleNull)
            {
                HandleNullOnRead = true;
                HandleNullOnWrite = true;
            }
            else if (UsesDefaultHandleNull)
            {
                // If the type doesn't support null, allow the converter a chance to modify.