ronantakizawa/codereview-bench huggingface dataset:
Repository: dotnet/roslyn
File: src/Features/Core/Portable/Diagnostics/Service/DiagnosticAnalyzerService.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.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Composition;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeStyle;
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using Microsoft.CodeAnalysis.SolutionCrawler;
using Microsoft.CodeAnalysis.Workspaces.Diagnostics;

namespace Microsoft.CodeAnalysis.Diagnostics;

[ExportWorkspaceServiceFactory(typeof(IDiagnosticAnalyzerService)), Shared]
[method: ImportingConstructor]
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal sealed class DiagnosticAnalyzerServiceFactory(
    IGlobalOptionService globalOptions,
    IDiagnosticsRefresher diagnosticsRefresher,
    [Import(AllowDefault = true)] IAsynchronousOperationListenerProvider? listenerProvider) : IWorkspaceServiceFactory
{
    public IWorkspaceService CreateService(HostWorkspaceServices workspaceServices)
    {
        return new DiagnosticAnalyzerService(
            globalOptions,

Review comment (suggestion):
```suggestion
using System.Composition;
using System.Linq;
```

Code after:
﻿// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Composition;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeStyle;
using Microsoft.CodeAnalysis.Host;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using Microsoft.CodeAnalysis.SolutionCrawler;
using Microsoft.CodeAnalysis.Workspaces.Diagnostics;

namespace Microsoft.CodeAnalysis.Diagnostics;

[ExportWorkspaceServiceFactory(typeof(IDiagnosticAnalyzerService)), Shared]
[method: ImportingConstructor]
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal sealed class DiagnosticAnalyzerServiceFactory(
    IGlobalOptionService globalOptions,
    IDiagnosticsRefresher diagnosticsRefresher,
    [Import(AllowDefault = true)] IAsynchronousOperationListenerProvider? listenerProvider) : IWorkspaceServiceFactory
{
    public IWorkspaceService CreateService(HostWorkspaceServices workspaceServices)
    {
        return new DiagnosticAnalyzerService(
Repository: dotnet/runtime
File: src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/ICertificatePal.cs (C#)

Code before:
using System.Collections.Generic;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using System.Text;

namespace Internal.Cryptography
{
    /// <summary>Provides specific implementation for X509Certificate2.</summary>
    internal interface ICertificatePal : ICertificatePalCore
    {
        int Version { get; }
        bool Archived { get; set; }
        string FriendlyName { get; set; }
        X500DistinguishedName SubjectName { get; }
        X500DistinguishedName IssuerName { get; }
        IEnumerable<X509Extension> Extensions { get; }
        RSA GetRSAPrivateKey();
        DSA GetDSAPrivateKey();
        ECDsa GetECDsaPrivateKey();
        string GetNameInfo(X509NameType nameType, bool forIssuer);
        void AppendPrivateKeyInfo(StringBuilder sb);
        ICertificatePal CopyWithPrivateKey(DSA privateKey);
        ICertificatePal CopyWithPrivateKey(ECDsa privateKey);
        ICertificatePal CopyWithPrivateKey(RSA privateKey);
        bool GetPolicyData(out byte[] ApplicationCertPoliciesData, out byte[] CertPoliciesData, out byte[] CertPolicyMappingsData,
                           out byte[] CertPolicyConstraintsData, out byte[] EnhancedKeyUsageData, out byte[] InhibitAnyPolicyExtensionData);
    }
}


Review comment (suggestion):
Parameter names should be camelCased, and please chop to one per line.

```C#
bool GetPolicyData(
    out byte[] applicationPolicies,
    out byte[] certPolicies,
    ...
    out byte[] inhibitAnyPolicy);
```

Code after:
using System.Collections.Generic;
using System.Runtime.InteropServices;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using System.Text;

namespace Internal.Cryptography
{
    [StructLayout(LayoutKind.Sequential)]
    internal struct PolicyData
    {
        internal byte[] ApplicationCertPolicies;
        internal byte[] CertPolicies;
        internal byte[] CertPolicyMappings;
        internal byte[] CertPolicyConstraints;
        internal byte[] EnhancedKeyUsage;
        internal byte[] InhibitAnyPolicyExtension;
    }

    /// <summary>Provides specific implementation for X509Certificate2.</summary>
    internal interface ICertificatePal : ICertificatePalCore
    {
        int Version { get; }
        bool Archived { get; set; }
        string FriendlyName { get; set; }
        X500DistinguishedName SubjectName { get; }
        X500DistinguishedName IssuerName { get; }
        IEnumerable<X509Extension> Extensions { get; }
        RSA GetRSAPrivateKey();
        DSA GetDSAPrivateKey();
        ECDsa GetECDsaPrivateKey();
        string GetNameInfo(X509NameType nameType, bool forIssuer);
        void AppendPrivateKeyInfo(StringBuilder sb);
        ICertificatePal CopyWithPrivateKey(DSA privateKey);
        ICertificatePal CopyWithPrivateKey(ECDsa privateKey);
        ICertificatePal CopyWithPrivateKey(RSA privateKey);
        PolicyData GetPolicyData();
    }
}
Repository: dotnet/runtime
File: src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs (C#)

Code before:

            public ValueTask WriteAsync(byte[] buffer, int offset, int count) => _sslStream.InnerStream.WriteAsync(new ReadOnlyMemory<byte>(buffer, offset, count), _cancellationToken);

            public async ValueTask LockAsync(TaskCompletionSource<bool> waiter)
            {
                await waiter.Task.ConfigureAwait(false);
            }

            public CancellationToken CancellationToken => _cancellationToken;
        }

        private readonly struct SyncSslIOAdapter : ISslIOAdapter
        {
            private readonly SslStream _sslStream;

            public SyncSslIOAdapter(SslStream sslStream) => _sslStream = sslStream;

            public ValueTask<int> ReadAsync(Memory<byte> buffer) => new ValueTask<int>(_sslStream.InnerStream.Read(buffer.Span));

            public ValueTask WriteAsync(byte[] buffer, int offset, int count)
            {
                _sslStream.InnerStream.Write(buffer, offset, count);
                return default;
            }

            public ValueTask LockAsync(TaskCompletionSource<bool> waiter)
            {
                waiter.Task.Wait();
                return default;
            }

            public CancellationToken CancellationToken => default;
        }
    }
}


Review comment (suggestion):
```suggestion
            public Task LockAsync(TaskCompletionSource<bool> waiter)
```

Code after:

            public ValueTask WriteAsync(byte[] buffer, int offset, int count) => _sslStream.InnerStream.WriteAsync(new ReadOnlyMemory<byte>(buffer, offset, count), _cancellationToken);

            public Task WaitAsync(TaskCompletionSource<bool> waiter) => waiter.Task;

            public CancellationToken CancellationToken => _cancellationToken;
        }

        private readonly struct SyncSslIOAdapter : ISslIOAdapter
        {
            private readonly SslStream _sslStream;

            public SyncSslIOAdapter(SslStream sslStream) => _sslStream = sslStream;

            public ValueTask<int> ReadAsync(Memory<byte> buffer) => new ValueTask<int>(_sslStream.InnerStream.Read(buffer.Span));

            public ValueTask WriteAsync(byte[] buffer, int offset, int count)
            {
                _sslStream.InnerStream.Write(buffer, offset, count);
                return default;
            }

            public Task WaitAsync(TaskCompletionSource<bool> waiter)
            {
                waiter.Task.Wait();
                return Task.CompletedTask;
            }

            public CancellationToken CancellationToken => default;
        }
    }
}
Repository: dotnet/runtime
File: src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs (C#)

Code before:
            public async ValueTask LockAsync(TaskCompletionSource<bool> waiter)
            {
                await waiter.Task.ConfigureAwait(false);
            }

            public CancellationToken CancellationToken => _cancellationToken;
        }

        private readonly struct SyncSslIOAdapter : ISslIOAdapter
        {
            private readonly SslStream _sslStream;

            public SyncSslIOAdapter(SslStream sslStream) => _sslStream = sslStream;

            public ValueTask<int> ReadAsync(Memory<byte> buffer) => new ValueTask<int>(_sslStream.InnerStream.Read(buffer.Span));

            public ValueTask WriteAsync(byte[] buffer, int offset, int count)
            {
                _sslStream.InnerStream.Write(buffer, offset, count);
                return default;
            }

            public ValueTask LockAsync(TaskCompletionSource<bool> waiter)
            {
                waiter.Task.Wait();
                return default;
            }

            public CancellationToken CancellationToken => default;
        }
    }
}


Review comment (suggestion):
```suggestion
                return Task.CompletedTask;
```

Code after:
            public Task WaitAsync(TaskCompletionSource<bool> waiter) => waiter.Task;

            public CancellationToken CancellationToken => _cancellationToken;
        }

        private readonly struct SyncSslIOAdapter : ISslIOAdapter
        {
            private readonly SslStream _sslStream;

            public SyncSslIOAdapter(SslStream sslStream) => _sslStream = sslStream;

            public ValueTask<int> ReadAsync(Memory<byte> buffer) => new ValueTask<int>(_sslStream.InnerStream.Read(buffer.Span));

            public ValueTask WriteAsync(byte[] buffer, int offset, int count)
            {
                _sslStream.InnerStream.Write(buffer, offset, count);
                return default;
            }

            public Task WaitAsync(TaskCompletionSource<bool> waiter)
            {
                waiter.Task.Wait();
                return Task.CompletedTask;
            }

            public CancellationToken CancellationToken => default;
        }
    }
}
Repository: dotnet/runtime
File: src/libraries/Native/Unix/System.Security.Cryptography.Native/apibridge.c (C)

Code before:
        return x509->cert_info->validity->notBefore;
    }

    return NULL;
}

const ASN1_TIME* local_X509_get0_notAfter(const X509* x509)
{
    if (x509 && x509->cert_info && x509->cert_info->validity)
    {
        return x509->cert_info->validity->notAfter;
    }

    return NULL;
}

int local_X509_set1_notBefore(X509* x509, const ASN1_TIME* time)
{
    if (x509 && x509->cert_info && x509->cert_info->validity)
    {
        if (x509->cert_info->validity->notBefore)
        {
            ASN1_TIME_free(x509->cert_info->validity->notBefore);
        }

        x509->cert_info->validity->notBefore = ASN1_STRING_dup(time);
        return x509->cert_info->validity->notBefore != NULL;
    }

    return 0;
}

int local_X509_set1_notAfter(X509* x509, const ASN1_TIME* time)
{
    if (x509 && x509->cert_info && x509->cert_info->validity)
    {
        if (x509->cert_info->validity->notAfter)
        {
            ASN1_TIME_free(x509->cert_info->validity->notAfter);
        }

        x509->cert_info->validity->notAfter = ASN1_STRING_dup(time);
        return x509->cert_info->validity->notAfter != NULL;
    }

    return 0;
}

const ASN1_TIME* local_X509_CRL_get0_nextUpdate(const X509_CRL* crl)
{
    if (crl && crl->crl)

Review comment (bug):
This should be wrapped in `if (x509->cert_info->validity->notBefore != time)`, both for side effect observability reasons, and to avoid `X509_set1_notBefore(cert, X509_get0_notBefore(cert))` from segfaulting.

Code after:
    {
        return x509->cert_info->validity->notBefore;
    }

    return NULL;
}

const ASN1_TIME* local_X509_get0_notAfter(const X509* x509)
{
    if (x509 && x509->cert_info && x509->cert_info->validity)
    {
        return x509->cert_info->validity->notAfter;
    }

    return NULL;
}

int local_X509_set1_notBefore(X509* x509, const ASN1_TIME* time)
{
    if (x509 && x509->cert_info && x509->cert_info->validity)
    {
        if (time != x509->cert_info->validity->notBefore)
        {
            if (x509->cert_info->validity->notBefore)
            {
                ASN1_TIME_free(x509->cert_info->validity->notBefore);
            }

            x509->cert_info->validity->notBefore = ASN1_STRING_dup(time);
        }

        return x509->cert_info->validity->notBefore != NULL;
    }

    return 0;
}

int local_X509_set1_notAfter(X509* x509, const ASN1_TIME* time)
{
    if (x509 && x509->cert_info && x509->cert_info->validity)
    {
        if (time != x509->cert_info->validity->notAfter)
        {
            if (x509->cert_info->validity->notAfter)
            {
                ASN1_TIME_free(x509->cert_info->validity->notAfter);
            }

            x509->cert_info->validity->notAfter = ASN1_STRING_dup(time);
        }
Repository: dotnet/runtime
File: src/libraries/Native/Unix/System.Security.Cryptography.Native/apibridge.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 "opensslshim.h"
#include "pal_crypto_types.h"
#include "pal_types.h"

#include "../Common/pal_safecrt.h"
#include <assert.h>

#ifdef NEED_OPENSSL_1_0

#include "apibridge.h"

// Minimally define the structs from 1.0.x which went opaque in 1.1.0 for the
// portable build building against the 1.1.x headers
#if OPENSSL_VERSION_NUMBER >= OPENSSL_VERSION_1_1_0_RTM
#include "openssl_1_0_structs.h"

#define CRYPTO_LOCK_X509 3
#define CRYPTO_LOCK_EVP_PKEY 10
#define CRYPTO_LOCK_BIO 21
#define SSL_CTRL_GET_SESSION_REUSED 8
#define SSL_CTRL_OPTIONS 32

#define SSL_ST_OK 3
#endif

c_static_assert(X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS == 4);

const ASN1_TIME* local_X509_get0_notBefore(const X509* x509)
{
    if (x509 && x509->cert_info && x509->cert_info->validity)
    {
        return x509->cert_info->validity->notBefore;
    }

    return NULL;
}

const ASN1_TIME* local_X509_get0_notAfter(const X509* x509)
{
    if (x509 && x509->cert_info && x509->cert_info->validity)
    {
        return x509->cert_info->validity->notAfter;
    }


Review comment (suggestion):
```suggestion
#define CRYPTO_LOCK_BIO 21

```

Please leave the separation between the crypto lock IDs and the SSL_CTRL IDs.

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 "opensslshim.h"
#include "pal_crypto_types.h"
#include "pal_types.h"

#include "../Common/pal_safecrt.h"
#include <assert.h>

#ifdef NEED_OPENSSL_1_0

#include "apibridge.h"

// Minimally define the structs from 1.0.x which went opaque in 1.1.0 for the
// portable build building against the 1.1.x headers
#if OPENSSL_VERSION_NUMBER >= OPENSSL_VERSION_1_1_0_RTM
#include "openssl_1_0_structs.h"

#define CRYPTO_LOCK_X509 3
#define CRYPTO_LOCK_EVP_PKEY 10
#define CRYPTO_LOCK_BIO 21

#define SSL_CTRL_GET_SESSION_REUSED 8
#define SSL_CTRL_OPTIONS 32

#define SSL_ST_OK 3
#endif

c_static_assert(X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS == 4);

const ASN1_TIME* local_X509_get0_notBefore(const X509* x509)
{
    if (x509 && x509->cert_info && x509->cert_info->validity)
    {
        return x509->cert_info->validity->notBefore;
    }

    return NULL;
}

const ASN1_TIME* local_X509_get0_notAfter(const X509* x509)
{
    if (x509 && x509->cert_info && x509->cert_info->validity)
    {
        return x509->cert_info->validity->notAfter;
    }
Repository: dotnet/runtime
File: src/libraries/System.Net.Security/src/System/Net/Security/SslClientAuthenticationOptions.cs (C#)

Code before:
                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):
```suggestion
        /// validation. If not <see langword="null"/>,
        /// <see cref="CertificateRevocationCheckMode"/> and <see cref="SslCertificateTrust"/>
        /// are ignored.
```

(updated to also change the bare words of the other properties to see-crefs)

Code after:
                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.Net.Security/src/System/Net/Security/SslClientAuthenticationOptions.cs (C#)

Code before:
                    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):
Properties are supposed to have both a summary and a value tag.

https://github.com/dotnet/dotnet-api-docs/wiki/Property-Value

```suggestion
        /// </summary>
        /// <value>
        ///   An optional customized policy for certificate chain validation.
        ///   The default is <see langword="null" />.
        /// </value>
        public X509ChainPolicy? ValidationPolicy { get; set; }
```

Code after:
                    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; }
    }
}