ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/PowerToys
File: src/modules/keyboardmanager/dll/dllmain.cpp (C++)

Code before:
    virtual bool is_enabled_by_default() const override
    {
        return false;
    }

    // Return the invocation hotkey for toggling
    virtual size_t get_hotkeys(Hotkey* hotkeys, size_t buffer_size) override
    {
        if (m_hotkey.key)
        {
            if (hotkeys && buffer_size >= 1)
            {
                hotkeys[0] = m_hotkey;
            }
            return 1;
        }
        else
        {
            return 0;
        }
    }

    // Process the hotkey event
    virtual bool on_hotkey(size_t /*hotkeyId*/) override
    {
        if (m_enabled)
        {
            disable();
            notifications::show_toast(L"Keyboard Manager Disabled", L"");
        }
        else
        {
            enable();
            notifications::show_toast(L"Keyboard Manager Enabled", L"");
        }
        return true;
    }
};

extern "C" __declspec(dllexport) PowertoyModuleIface* __cdecl powertoy_create()
{
    return new KeyboardManager();
}

Review comment (suggestion):
We currently only use notifications to communicate updates and other 'critical' things as we know users don't like to get too many notifications.

Since enabling/disabling KMB is a deliberate action by the user, notifications seem excessive.

Code after:
        HINSTANCE hinstance = reinterpret_cast<HINSTANCE>(&__ImageBase);

        // Create a Settings object.
        PowerToysSettings::Settings settings(hinstance, get_name());
        settings.set_description(IDS_SETTINGS_DESCRIPTION);
        settings.set_overview_link(L"https://aka.ms/PowerToysOverview_KeyboardManager");

        return settings.serialize_to_buffer(buffer, buffer_size);
    }

    // Signal from the Settings editor to call a custom action.
    virtual void call_custom_action(const wchar_t* /*action*/) override
    {
    }

    // Called by the runner to pass the updated settings values as a serialized JSON.
    virtual void set_config(const wchar_t* config) override
    {
        try
        {
            // Parse the input JSON string.
            PowerToysSettings::PowerToyValues values =
                PowerToysSettings::PowerToyValues::from_json_string(config, get_key());
            parse_hotkey(values);

            // If you don't need to do any custom processing of the settings, proceed
            // to persists the values calling:
            values.save_to_settings_file();
        }
        catch (std::exception&)
        {
            // Improper JSON.
        }
    }

    // Enable the powertoy
    virtual void enable()
    {
        m_enabled = true;
        // Log telemetry
        Trace::EnableKeyboardManager(true);
        start_engine();
    }

    // Disable the powertoy
    virtual void disable()
    {
        m_enabled = false;
        // Log telemetry
        Trace::EnableKeyboardManager(false);
        stop_engine();
Repository: microsoft/PowerToys
File: src/modules/cmdpal/Microsoft.CmdPal.UI/Converters/GridItemContainerStyleSelector.cs (C#)

Code before:
{
    public IGridPropertiesViewModel? GridProperties { get; set; }

    public Style? Small { get; set; }

    public Style? Medium { get; set; }

    public Style? Gallery { get; set; }

    public Style? Section { get; set; }

    public Style? Separator { get; set; }

    protected override Style? SelectStyleCore(object item, DependencyObject container)
    {
        return item switch
        {
            ListItemViewModel { IsSectionOrSeparator: true } listItemViewModel when string.IsNullOrWhiteSpace(listItemViewModel.Title) => Separator!,
            ListItemViewModel { IsSectionOrSeparator: true } => Section,
            _ => GridProperties switch
            {
                SmallGridPropertiesViewModel => Small,
                MediumGridPropertiesViewModel => Medium,
                GalleryGridPropertiesViewModel => Gallery,
                _ => Medium,
            },
        };
    }
}


Review comment (suggestion):
I'd suggest a slight tweak here to make this more readable and to separate out the nested elements. How about:

```
// Handle section/separator cases first.
if (item is ListItemViewModel { IsSectionOrSeparator: true } listItem)
{
    return string.IsNullOrWhiteSpace(listItem.Title) 
        ? Separator! 
        : Section;
}

// Otherwise, determine size from grid properties.
return GridProperties switch
{
    SmallGridPropertiesViewModel => Small,
    MediumGridPropertiesViewModel => Medium,
    GalleryGridPropertiesViewModel => Gallery,
    _ => Medium,
}
```

Code after:
{
    public IGridPropertiesViewModel? GridProperties { get; set; }

    public Style? Small { get; set; }

    public Style? Medium { get; set; }

    public Style? Gallery { get; set; }

    public Style? Section { get; set; }

    public Style? Separator { get; set; }

    protected override Style? SelectStyleCore(object item, DependencyObject container)
    {
        if (item is ListItemViewModel { IsSectionOrSeparator: true } listItem)
        {
            return string.IsNullOrWhiteSpace(listItem.Title)
                ? Separator!
                : Section;
        }

        return GridProperties switch
        {
            SmallGridPropertiesViewModel => Small,
            MediumGridPropertiesViewModel => Medium,
            GalleryGridPropertiesViewModel => Gallery,
            _ => Medium,
        };
    }
}
Repository: microsoft/PowerToys
File: tools/build/setup-dev-environment.ps1 (PowerShell)

Code before:

.PARAMETER SkipDevMode
Skip enabling Windows Developer Mode.

.PARAMETER SkipVSComponents
Skip installing Visual Studio components from .vsconfig.

.PARAMETER SkipSubmodules
Skip initializing git submodules.

.PARAMETER VSInstallPath
Path to Visual Studio installation. Default: auto-detected.

.PARAMETER Help
Show this help message.

.EXAMPLE
.\tools\build\setup-dev-environment.ps1
Runs the full setup process.

.EXAMPLE
.\tools\build\setup-dev-environment.ps1 -SkipVSComponents
Runs setup but skips Visual Studio component installation.

.EXAMPLE
.\tools\build\setup-dev-environment.ps1 -VSInstallPath "C:\Program Files\Microsoft Visual Studio\2026\Enterprise"
Runs setup with a custom Visual Studio installation path.

.NOTES
- Some operations require administrator privileges (long paths, VS component installation).
- If not running as administrator, the script will prompt for elevation for those steps.
- The script is idempotent and safe to run multiple times.
#>

param (
    [switch]$SkipLongPaths,
    [switch]$SkipDevMode,
    [switch]$SkipVSComponents,
    [switch]$SkipSubmodules,
    [string]$VSInstallPath = '',
    [switch]$Help
)

if ($Help) {
    Get-Help $MyInvocation.MyCommand.Path -Detailed
    exit 0
}

$ErrorActionPreference = 'Stop'

# Find repository root

Review comment (suggestion):
```suggestion
.\tools\build\setup-dev-environment.ps1 -VSInstallPath "C:\Program Files\Microsoft Visual Studio\18\Enterprise"
```

Code after:

.PARAMETER SkipDevMode
Skip enabling Windows Developer Mode.

.PARAMETER SkipVSComponents
Skip installing Visual Studio components from .vsconfig.

.PARAMETER SkipSubmodules
Skip initializing git submodules.

.PARAMETER VSInstallPath
Path to Visual Studio installation. Default: auto-detected.

.PARAMETER Help
Show this help message.

.EXAMPLE
.\tools\build\setup-dev-environment.ps1
Runs the full setup process.

.EXAMPLE
.\tools\build\setup-dev-environment.ps1 -SkipVSComponents
Runs setup but skips Visual Studio component installation.

.EXAMPLE
.\tools\build\setup-dev-environment.ps1 -VSInstallPath "C:\Program Files\Microsoft Visual Studio\18\Enterprise"
Runs setup with a custom Visual Studio installation path.

.NOTES
- Some operations require administrator privileges (long paths, VS component installation).
- If not running as administrator, the script will prompt for elevation for those steps.
- The script is idempotent and safe to run multiple times.
#>

param (
    [switch]$SkipLongPaths,
    [switch]$SkipDevMode,
    [switch]$SkipVSComponents,
    [switch]$SkipSubmodules,
    [string]$VSInstallPath = '',
    [switch]$Help
)

if ($Help) {
    Get-Help $MyInvocation.MyCommand.Path -Detailed
    exit 0
}

$ErrorActionPreference = 'Stop'

# Find repository root
Repository: microsoft/TypeScript
File: tests/cases/compiler/accessorDeclarationEmitJs.ts (TypeScript)

Code before:
// @allowJs: true
// @checkJs: true
// @declaration: true
// @emitDeclarationOnly: true
// @filename: /a.js
export const object = {
    property: 'value',
    get getter() {
        return 'value';
    },
}


Review comment (suggestion):
```suggestion
}

export const object2 = {
    property: 'value',
    set setter(v) {
    },
}
```

Code after:
// @allowJs: true
// @checkJs: true
// @declaration: true
// @emitDeclarationOnly: true
// @filename: /a.js
export const t1 = {
    p: 'value',
    get getter() {
        return 'value';
    },
}

export const t2 = {
    v: 'value',
    set setter(v) {},
}

export const t3 = {
    p: 'value',
    get value() {
        return 'value';
    },
    set value(v) {},
}
Repository: microsoft/TypeScript
File: tests/cases/fourslash/codeFixCorrectReturnValue28.ts (TypeScript)

Code before:
/// <reference path='fourslash.ts' />

// @strict: true

// @Filename: test.ts
//// (function() {
////   const config = {
////     values: [],
////     value: {},
////   };
////
////   config.values.push(config.value);
//// })();

goTo.file('test.ts');

verify.getSemanticDiagnostics([{
    code: 2345,
    message: "Argument of type '{}' is not assignable to parameter of type 'never'.",
    range: { pos: 91, end: 103,fileName: "test.ts" }
}]);

verify.not.codeFixAvailable();


Review comment (bug):
```suggestion
//// }();
```
The test, as it currently is, doesn't reproduce the crash. However, the code in the original report, or enclosing the parens like `(function () { /* ... */ }());`, will

Code after:
/// <reference path='fourslash.ts' />

// @strict: true

// @Filename: test.ts
//// (function() {
////   const config = {
////     values: [],
////     value: {},
////   };
////
////   config.values.push(config.value);
//// }());

goTo.file('test.ts');

verify.getSemanticDiagnostics([{
    code: 2345,
    message: "Argument of type '{}' is not assignable to parameter of type 'never'.",
    range: { pos: 91, end: 103,fileName: "test.ts" }
}]);

verify.not.codeFixAvailable();
Repository: microsoft/TypeScript
File: scripts/build/tests.mjs (JavaScript)

Code before:
        }
        else {
            args.push("-t", "" + testTimeout);
        }
        args.push(runJs);
    }
    else {
        // run task to load all tests and partition them between workers
        args.push(runJs);
    }

    /** @type {number | undefined} */
    let errorStatus;

    /** @type {Error | undefined} */
    let error;

    try {
        setNodeEnvToDevelopment();

        const { exitCode } = await exec(process.execPath, args, { token: options.token });
        if (exitCode !== 0) {
            errorStatus = exitCode;
            error = new Error(`Process exited with status code ${errorStatus}.`);
        }
    }
    catch (e) {
        errorStatus = undefined;
        error = /** @type {Error} */ (e);
    }
    finally {
        restoreSavedNodeEnv();
    }

    await del("test.config");
    await deleteTemporaryProjectOutput();

    if (error !== undefined) {
        if (error instanceof CancelError) {
            throw error;
        }

        if (options.watching) {
            console.error(`${chalk.redBright(error.name)}: ${error.message}`);
        }
        else {
            process.exitCode = typeof errorStatus === "number" ? errorStatus : 2;
            throw error;
        }
    }
}

Review comment (suggestion):
Is there a benefit from doing this rather than letting `c8` process it? I didn't know that `c8 report` existed before now (I had always run it as a node wrapper).

Code after:
        if (inspect !== undefined) {
            args.unshift((inspect === "" || inspect === true) ? "--inspect-brk" : "--inspect-brk="+inspect);
            args.push("-t", "0");
        }
        else {
            args.push("-t", "" + testTimeout);
        }
        args.push(runJs);
    }
    else {
        // run task to load all tests and partition them between workers
        args.push(runJs);
    }

    /** @type {number | undefined} */
    let errorStatus;

    /** @type {Error | undefined} */
    let error;

    const savedNodeEnv = process.env.NODE_ENV;
    const savedNodeV8Coverage = process.env.NODE_V8_COVERAGE;
    try {
        process.env.NODE_ENV = "development";
        if (coverage) {
            process.env.NODE_V8_COVERAGE = path.resolve(coverageDir, "tmp");
        }

        await exec(process.execPath, args, { token: options.token });
        if (coverage) {
            await exec("npm", ["--prefer-offline", "exec", "--", "c8", "report"], { token: options.token });
        }
    }
    catch (e) {
        errorStatus = e instanceof ExecError ? e.exitCode ?? undefined : undefined;
        error = /** @type {Error} */ (e);
    }
    finally {
        if (coverage) {
            process.env.NODE_V8_COVERAGE = savedNodeV8Coverage;
        }
        process.env.NODE_ENV = savedNodeEnv;
    }

    await del("test.config");
    await deleteTemporaryProjectOutput();

    if (error !== undefined) {
        if (error instanceof CancelError) {
            throw error;
        }
Repository: microsoft/TypeScript
File: src/services/codefixes/convertTypedefToType.ts (TypeScript)

Code before:
            typeReference = getSynthesizedDeepClone(type);
        }

        if (typeReference && name) {
            const questionToken = isOptional ? factory.createToken(SyntaxKind.QuestionToken) : undefined;

            return factory.createPropertySignature(
                /*modifiers*/ undefined,
                name,
                questionToken,
                typeReference
            );
        }
    };

    return mapDefined(propertyTags, getSignature);
}

function getPropertyName(tag: JSDocPropertyLikeTag): string | undefined {
    return tag.name.kind === SyntaxKind.Identifier ? tag.name.text : tag.name.right.text;
}

/** @internal */
export function getJSDocTypedefNodes(node: Node): readonly JSDocTag[] {
    if (hasJSDocNodes(node)) {
        return flatMapToMutable(node.jsDoc, (doc) => doc.tags?.filter((tag) => isJSDocTypedefTag(tag)));
    }

    return [];
}


Review comment (performance):
Could this just be `flatMap`? There's only one use and it doesn't actually need a copy, so `flatMap` should be more efficient.

Code after:

    return factory.createInterfaceDeclaration(
        /*modifiers*/ undefined,
        typeName,
        /*typeParameters*/ undefined,
        /*heritageClauses*/ undefined,
        propertySignatures,
    );
}

function createTypeAliasForTypeExpression(
    typeName: string,
    typeExpression: JSDocTypeExpression
): TypeAliasDeclaration | undefined {
    const typeReference = getSynthesizedDeepClone(typeExpression.type);
    if (!typeReference) return;

    return factory.createTypeAliasDeclaration(
        /*modifiers*/ undefined,
        factory.createIdentifier(typeName),
        /*typeParameters*/ undefined,
        typeReference
    );
}

function createSignatureFromTypeLiteral(typeLiteral: JSDocTypeLiteral): PropertySignature[] | undefined {
    const propertyTags = typeLiteral.jsDocPropertyTags;
    if (!some(propertyTags)) return;

    const getSignature = (tag: JSDocPropertyLikeTag) => {
        const name = getPropertyName(tag);
        const type = tag.typeExpression?.type;
        const isOptional = tag.isBracketed;
        let typeReference;

        // Recursively handle nested object type
        if (type && isJSDocTypeLiteral(type)) {
            const signatures = createSignatureFromTypeLiteral(type);
            typeReference = factory.createTypeLiteralNode(signatures);
        }
        // Leaf node, where type.kind === SyntaxKind.JSDocTypeExpression
        else if (type) {
            typeReference = getSynthesizedDeepClone(type);
        }

        if (typeReference && name) {
            const questionToken = isOptional ? factory.createToken(SyntaxKind.QuestionToken) : undefined;

            return factory.createPropertySignature(
                /*modifiers*/ undefined,
                name,
Repository: microsoft/TypeScript
File: tests/cases/compiler/expandoFunctionNullishProperty.ts (TypeScript)

Code before:
// @strict: true
// @target: esnext
// @declaration: true

// mentioned in https://github.com/microsoft/TypeScript/issues/54220

interface TestNull {
  (): void;
  readonly prop: null;
}

export function testNull(): TestNull {
  function inner() {}
  inner.prop = null;
  return inner;
}

interface TestUndefined {
  (): void;
  readonly prop: undefined;
}

export function testUndefined(): TestUndefined {
  function inner() {}
  inner.prop = undefined;
  return inner;
}


Review comment (suggestion):
please add a test that shows `inner` is assignable to a more realistic type like

```ts
interface TestStringNull {
  (): void;
  prop: string | null
}
```

Since `readonly prop: null` is intentionally useless (and why the JS rule existed in the first place).

Code after:
// @strict: true
// @target: esnext
// @declaration: true

// mentioned in https://github.com/microsoft/TypeScript/issues/54220

interface TestNull {
  (): void;
  readonly prop: null;
}

export function testNull(): TestNull {
  function inner() {}
  inner.prop = null;
  return inner;
}

interface TestNull2 {
  (): void;
  prop: string | null;
}

export function testNull2(): TestNull2 {
  function inner() {}
  inner.prop = null;
  return inner;
}

interface TestUndefined {
  (): void;
  readonly prop: undefined;
}

export function testUndefined(): TestUndefined {
  function inner() {}
  inner.prop = undefined;
  return inner;
}
Repository: microsoft/TypeScript
File: tests/cases/compiler/typeAliasDeclareKeywordNewlines.ts (TypeScript)

Code before:
var declare: any, type: any;

// The following is invalid but should declare a type alias named 'T1':
declare type /*unexpected newline*/
T1 = null;
const t1: T1 = null;

let T: null;
// The following should use a variable named 'declare', use a variable named
// 'type', and assign to a variable named 'T'.
declare /*ASI*/
type /*ASI*/
T = null;

// The following should use a variable named 'declare' and declare a type alias
// named 'T2':
declare /*ASI*/
type T2 = null;
const t2: T2 = null;


Review comment (suggestion):
Consider giving these distinct types

```suggestion
var declare: string, type: number;
```

Code after:
var declare: string, type: number;

// The following is invalid but should declare a type alias named 'T1':
declare type /*unexpected newline*/
T1 = null;
const t1: T1 = null; // Assert that T1 is the null type.

let T: null;
// The following should use a variable named 'declare', use a variable named
// 'type', and assign to a variable named 'T'.
declare /*ASI*/
type /*ASI*/
T = null;

// The following should use a variable named 'declare' and declare a type alias
// named 'T2':
declare /*ASI*/
type T2 = null;
const t2: T2 = null; // Assert that T2 is the null type.
Repository: microsoft/TypeScript
File: tests/baselines/reference/castExpressionParentheses.js (JavaScript)

Code before:

//// [castExpressionParentheses.js]
// parentheses should be omitted
// literals
({ a: 0 });
[1, 3,];
"string";
23.0;
1;
1.;
1.0;
12e+34;
0xff;
/regexp/g;
false;
true;
null;
// names and dotted names
this;
this.x;
a.x;
a;
a[0];
a.b["0"];
a().x;
(1).foo;
(1.).foo;
(1.0).foo;
12e+34.foo;
0xff.foo;
// should keep the parentheses in emit
(1.0);
(new A).foo;
(typeof A).x;
(-A).x;
new (A());
(function () { })();
(function foo() { })();
(-A).x;
// nested cast, should keep one pair of parenthese
(-A).x;
// nested parenthesized expression, should keep one pair of parenthese
(A);


Review comment (suggestion):
This PR is not fully ready - it requires just small tweaks. I would like to get the TS team opinion as to what is preferred first. 

I think that the whole double dot thing could likely just be replaced with parens (I think that @jakebailey might agree with me on this one: https://github.com/microsoft/TypeScript/pull/55031#issuecomment-1640562028 ). I could just add parenthesis for negative numbers and double dot for positive ones but honestly, that just sounds like a redundant complication to me.



Code after:

//// [castExpressionParentheses.js]
// parentheses should be omitted
// literals
({ a: 0 });
[1, 3,];
"string";
23.0;
1;
1.;
1.0;
12e+34;
0xff;
/regexp/g;
false;
true;
null;
// names and dotted names
this;
this.x;
a.x;
a;
a[0];
a.b["0"];
a().x;
1..foo;
1..foo;
1.0.foo;
12e+34.foo;
0xff.foo;
// should keep the parentheses in emit
(1.0);
(new A).foo;
(typeof A).x;
(-A).x;
new (A());
(function () { })();
(function foo() { })();
(-A).x;
// nested cast, should keep one pair of parenthese
(-A).x;
// nested parenthesized expression, should keep one pair of parenthese
(A);
Repository: microsoft/TypeScript
File: tests/baselines/reference/constEnumToStringWithComments.js (JavaScript)

Code before:
    Z = 2.,
    A = -1,
    B = -1.5,
    C = -1.
}

let x0 = Foo.X.toString();
let x1 = Foo["X"].toString();
let y0 = Foo.Y.toString();
let y1 = Foo["Y"].toString();
let z0 = Foo.Z.toString();
let z1 = Foo["Z"].toString();
let a0 = Foo.A.toString();
let a1 = Foo["A"].toString();
let b0 = Foo.B.toString();
let b1 = Foo["B"].toString();
let c0 = Foo.C.toString();
let c1 = Foo["C"].toString();


//// [constEnumToStringWithComments.js]
var x0 = (100 /* Foo.X */).toString();
var x1 = (100 /* Foo["X"] */).toString();
var y0 = 0.5 /* Foo.Y */.toString();
var y1 = 0.5 /* Foo["Y"] */.toString();
var z0 = (2 /* Foo.Z */).toString();
var z1 = (2 /* Foo["Z"] */).toString();
var a0 = (-1 /* Foo.A */).toString();
var a1 = (-1 /* Foo["A"] */).toString();
var b0 = (-1.5 /* Foo.B */).toString();
var b1 = (-1.5 /* Foo["B"] */).toString();
var c0 = (-1 /* Foo.C */).toString();
var c1 = (-1 /* Foo["C"] */).toString();


Review comment (suggestion):
`2.toString()` is invalid but `2/**/.toString()` is so those parentheses could also be avoided here, OTOH I'm not really sure if it's worth avoiding them here 🤷‍♂️ 

Code after:
    Z = 2.,
    A = -1,
    B = -1.5,
    C = -1.
}

let x0 = Foo.X.toString();
let x1 = Foo["X"].toString();
let y0 = Foo.Y.toString();
let y1 = Foo["Y"].toString();
let z0 = Foo.Z.toString();
let z1 = Foo["Z"].toString();
let a0 = Foo.A.toString();
let a1 = Foo["A"].toString();
let b0 = Foo.B.toString();
let b1 = Foo["B"].toString();
let c0 = Foo.C.toString();
let c1 = Foo["C"].toString();


//// [constEnumToStringWithComments.js]
var x0 = 100 /* Foo.X */.toString();
var x1 = 100 /* Foo["X"] */.toString();
var y0 = 0.5 /* Foo.Y */.toString();
var y1 = 0.5 /* Foo["Y"] */.toString();
var z0 = 2 /* Foo.Z */.toString();
var z1 = 2 /* Foo["Z"] */.toString();
var a0 = (-1 /* Foo.A */).toString();
var a1 = (-1 /* Foo["A"] */).toString();
var b0 = (-1.5 /* Foo.B */).toString();
var b1 = (-1.5 /* Foo["B"] */).toString();
var c0 = (-1 /* Foo.C */).toString();
var c1 = (-1 /* Foo["C"] */).toString();