ronantakizawa/codereview-bench huggingface dataset:
Repository: pixijs/pixijs
File: src/scene/text/canvas/CanvasTextSystem.ts (TypeScript)

Code before:
        {
            const currentChar = stringArray[i];

            if (isStroke)
            {
                context.strokeText(currentChar, currentPosition, y);
            }
            else
            {
                context.fillText(currentChar, currentPosition, y);
            }
            let textStr = '';

            for (let j = i + 1; j < stringArray.length; ++j)
            {
                textStr += stringArray[j];
            }
            currentWidth = context.measureText(textStr).width;
            currentPosition += previousWidth - currentWidth + letterSpacing;
            previousWidth = currentWidth;
        }
    }

    public destroy(): void
    {
        // BOOM!
    }
}


Review comment (suggestion):
The placeholder comment '// BOOM!' in destroy() does not provide clear cleanup instructions. Please implement proper resource disposal logic to ensure safe cleanup of allocated textures and contexts.
```suggestion
        // Clear allocated textures
        TexturePool.clear();

        // Clear allocated canvases
        CanvasPool.clear();

        // Add any additional cleanup logic here
```

Code after:
        {
            const currentChar = stringArray[i];

            if (isStroke)
            {
                context.strokeText(currentChar, currentPosition, y);
            }
            else
            {
                context.fillText(currentChar, currentPosition, y);
            }
            let textStr = '';

            for (let j = i + 1; j < stringArray.length; ++j)
            {
                textStr += stringArray[j];
            }
            currentWidth = context.measureText(textStr).width;
            currentPosition += previousWidth - currentWidth + letterSpacing;
            previousWidth = currentWidth;
        }
    }

    public destroy(): void
    {
        (this._renderer as null) = null;
    }
}
Repository: pixijs/pixijs
File: src/scene/text/canvas/CanvasTextSystem.ts (TypeScript)

Code before:
        {
            const currentChar = stringArray[i];

            if (isStroke)
            {
                context.strokeText(currentChar, currentPosition, y);
            }
            else
            {
                context.fillText(currentChar, currentPosition, y);
            }
            let textStr = '';

            for (let j = i + 1; j < stringArray.length; ++j)
            {
                textStr += stringArray[j];
            }
            currentWidth = context.measureText(textStr).width;
            currentPosition += previousWidth - currentWidth + letterSpacing;
            previousWidth = currentWidth;
        }
    }

    public destroy(): void
    {
        // BOOM!
    }
}


Review comment (suggestion):
```suggestion
        (this._renderer as null) = null;
```

Code after:
        {
            const currentChar = stringArray[i];

            if (isStroke)
            {
                context.strokeText(currentChar, currentPosition, y);
            }
            else
            {
                context.fillText(currentChar, currentPosition, y);
            }
            let textStr = '';

            for (let j = i + 1; j < stringArray.length; ++j)
            {
                textStr += stringArray[j];
            }
            currentWidth = context.measureText(textStr).width;
            currentPosition += previousWidth - currentWidth + letterSpacing;
            previousWidth = currentWidth;
        }
    }

    public destroy(): void
    {
        (this._renderer as null) = null;
    }
}
Repository: pixijs/pixijs
File: tests/visual/scenes/graphics/graphics-gradients.scene.ts (TypeScript)

Code before:
            colorStops,
            textureSpace: 'local',
        });
        const graphic4 = new Graphics()
            .rect(width + (spacing * 2), height + (spacing * 2), width, height)
            .fill(gradient4);

        const container = new Container();

        container.addChild(graphic1, graphic2, graphic3, graphic4);

        const bounds = container.getBounds();

        container.width = 128;
        container.height = 128;

        const scaleX = 128 / bounds.width;
        const scaleY = 128 / bounds.height;

        //  container.scale.set(scaleX, scaleY);
        container.x = -bounds.x * scaleX;
        container.y = -bounds.y * scaleY;

        scene.addChild(container);

        scene.addChild(container);
    },
};


Review comment (refactor):
The container is added twice to the scene; remove the duplicate call to avoid any unintended rendering side effects.
```suggestion

```

Code after:
            colorStops,
            textureSpace: 'local',
        });
        const graphic4 = new Graphics()
            .rect(width + (spacing * 2), height + (spacing * 2), width, height)
            .fill(gradient4);

        const container = new Container();

        container.addChild(graphic1, graphic2, graphic3, graphic4);

        const bounds = container.getBounds();

        container.width = 128;
        container.height = 128;

        const scaleX = 128 / bounds.width;
        const scaleY = 128 / bounds.height;

        //  container.scale.set(scaleX, scaleY);
        container.x = -bounds.x * scaleX;
        container.y = -bounds.y * scaleY;

        scene.addChild(container);
    },
};
Repository: pixijs/pixijs
File: src/scene/container/container-mixins/childrenHelperMixin.ts (TypeScript)

Code before:
            for (let i = end - 1; i >= beginIndex; i--)
            {
                const child = this.children[i];

                if (!child) continue;
                removed.push(child);
                child.parent = null;
            }

            removeItems(this.children, beginIndex, end);

            const renderGroup = this.renderGroup || this.parentRenderGroup;

            if (renderGroup)
            {
                renderGroup.removeChildren(removed);
            }

            for (let i = 0; i < removed.length; ++i)
            {
                const child = removed[i];

                if (child.parentRenderLayer)
                {
                    child.parentRenderLayer.detach(child);
                }

                this.emit('childRemoved', child, this, i);
                removed[i].emit('removed', this);
            }

            if (removed.length > 0)
            {
                this._didViewChangeTick++;
            }

            return removed;
        }
        else if (range === 0 && this.children.length === 0)
        {
            return removed;
        }

        throw new RangeError('removeChildren: numeric values are outside the acceptable range.');
    },

    /**
     * Removes a child from the specified index position.
     * @param index - The index to get the child from
     * @returns The child that was removed.
     * @memberof scene.Container#

Review comment (suggestion):
```suggestion
                child.parentRenderLayer?.detach(child);
```

Code after:
            for (let i = end - 1; i >= beginIndex; i--)
            {
                const child = this.children[i];

                if (!child) continue;
                removed.push(child);
                child.parent = null;
            }

            removeItems(this.children, beginIndex, end);

            const renderGroup = this.renderGroup || this.parentRenderGroup;

            if (renderGroup)
            {
                renderGroup.removeChildren(removed);
            }

            for (let i = 0; i < removed.length; ++i)
            {
                const child = removed[i];

                child.parentRenderLayer?.detach(child);

                this.emit('childRemoved', child, this, i);
                removed[i].emit('removed', this);
            }

            if (removed.length > 0)
            {
                this._didViewChangeTick++;
            }

            return removed;
        }
        else if (range === 0 && this.children.length === 0)
        {
            return removed;
        }

        throw new RangeError('removeChildren: numeric values are outside the acceptable range.');
    },

    /**
     * Removes a child from the specified index position.
     * @param index - The index to get the child from
     * @returns The child that was removed.
     * @memberof scene.Container#
     */
    removeChildAt<U extends(ContainerChild | IRenderLayer)>(index: number): U
    {
Repository: pixijs/pixijs
File: tests/visual/scenes/text/bitmap-text-dynamic.scene.ts (TypeScript)

Code before:
import { Assets } from '~/assets';
import { BitmapText } from '~/scene';

import type { TestScene } from '../../types';
import type { Container } from '~/scene';

export const scene: TestScene = {
    it: 'should render dynamic bitmap font with same style',
    create: async (scene: Container) =>
    {
        await Assets.load('fonts/outfit.woff2');

        const style = { fontFamily: 'Arial', fontSize: 30 };

        const text1 = new BitmapText({ text: 'abcdefgh', style });

        const text2 = new BitmapText({
            text: ' ijk', /* remove the whitespace and it's fine */
            style, y: 40,
        });

        scene.addChild(text1);
        scene.addChild(text2);
    },
};


Review comment (suggestion):
[nitpick] Clarify the purpose of the leading whitespace in the test case or remove the comment if it is no longer necessary to avoid reader confusion.
```suggestion
            text: ' ijk', // The leading whitespace is intentional to test how BitmapText handles it.
```

Code after:
import { Assets } from '~/assets';
import { BitmapText } from '~/scene';

import type { TestScene } from '../../types';
import type { Container } from '~/scene';

export const scene: TestScene = {
    it: 'should render dynamic bitmap font with same style',
    create: async (scene: Container) =>
    {
        await Assets.load('fonts/outfit.woff2');

        const style = { fontFamily: 'Arial', fontSize: 30 };

        const text1 = new BitmapText({ text: 'abcdefgh', style });

        const text2 = new BitmapText({
            text: ' ijk', // The leading whitespace is intentional to test how BitmapText handles it.
            style, y: 40,
        });

        scene.addChild(text1);
        scene.addChild(text2);
    },
};
Repository: pixijs/pixijs
File: src/rendering/batcher/gpu/getTextureBatchBindGroup.ts (TypeScript)

Code before:
import { BindGroup } from '../../renderers/gpu/shader/BindGroup';
import { Texture } from '../../renderers/shared/texture/Texture';
import { getMaxTexturesPerBatch } from '../gl/utils/maxRecommendedTextures';

import type { TextureSource } from '../../renderers/shared/texture/sources/TextureSource';

const cachedGroups: Record<number, BindGroup> = {};

export function getTextureBatchBindGroup(textures: TextureSource[], size: number)
{
    let uid = 2166136261; // FNV-1a 32-bit offset basis

    for (let i = 0; i < size; i++)
    {
        uid ^= textures[i].uid;
        uid = Math.imul(uid, 16777619);
        uid >>>= 0;
    }

    return cachedGroups[uid] || generateTextureBatchBindGroup(textures, size, uid);
}

let maxTextures = 0;

function generateTextureBatchBindGroup(textures: TextureSource[], size: number, key: number): BindGroup
{
    const bindGroupResources: Record<string, any> = {};

    let bindIndex = 0;

    if (!maxTextures)maxTextures = getMaxTexturesPerBatch();


Review comment (performance):
Please update or add a function doc comment to describe the new maxTextures parameter so that its purpose and usage are clear to future maintainers.
```suggestion

/**
 * Retrieves or generates a bind group for a batch of textures.
 *
 * @param textures - An array of texture sources to include in the bind group.
 * @param size - The number of textures in the batch.
 * @param maxTextures - The maximum number of textures that can be included in the bind group.
 *                     If the batch size is smaller than maxTextures, empty textures will be used to pad the group.
 * @returns The bind group corresponding to the provided textures.
 */
```

Code after:
import { BindGroup } from '../../renderers/gpu/shader/BindGroup';
import { Texture } from '../../renderers/shared/texture/Texture';

import type { TextureSource } from '../../renderers/shared/texture/sources/TextureSource';

const cachedGroups: Record<number, BindGroup> = {};

export function getTextureBatchBindGroup(textures: TextureSource[], size: number, maxTextures: number)
{
    let uid = 2166136261; // FNV-1a 32-bit offset basis

    for (let i = 0; i < size; i++)
    {
        uid ^= textures[i].uid;
        uid = Math.imul(uid, 16777619);
        uid >>>= 0;
    }

    return cachedGroups[uid] || generateTextureBatchBindGroup(textures, size, uid, maxTextures);
}

function generateTextureBatchBindGroup(textures: TextureSource[], size: number, key: number, maxTextures: number): BindGroup
{
    const bindGroupResources: Record<string, any> = {};

    let bindIndex = 0;

    for (let i = 0; i < maxTextures; i++)
    {
        const texture = i < size ? textures[i] : Texture.EMPTY.source;

        bindGroupResources[bindIndex++] = texture.source;
Repository: pixijs/pixijs
File: tests/visual/scenes/mask/trimed-sprite-mask.scene.ts (TypeScript)

Code before:
            .rect(0, 0, 100, 100)
            .fill(new Color('red'));

        rect.width = 100;
        rect.height = 100;

        const texture = await Assets.load('blurredCircle.png') as Texture;
        const trimedTexture = new Texture({
            source: texture.source,
            frame: new Rectangle(0, 0, 200, 200),
            orig: new Rectangle(0, 0, 400, 300),
            trim: new Rectangle(100, 100, 200, 200)
        });
        const mask = new Sprite(trimedTexture);

        mask.width = 40;
        mask.height = 30;

        mask.anchor.set(0.5);

        mask.position.set(128 / 2);

        rect.mask = mask;
        rect.setMask({
            inverse: true,
        });

        scene.addChild(rect);
        scene.addChild(mask);
    },
};


Review comment (refactor):
Can simplify:
```suggestion
        rect.setMask({
            mask,
            inverse: true,
        });
```


Code after:
            .rect(0, 0, 100, 100)
            .fill(new Color('red'));

        rect.width = 100;
        rect.height = 100;

        const texture = await Assets.load('blurredCircle.png') as Texture;
        const trimedTexture = new Texture({
            source: texture.source,
            frame: new Rectangle(0, 0, 200, 200),
            orig: new Rectangle(0, 0, 400, 300),
            trim: new Rectangle(100, 100, 200, 200)
        });
        const mask = new Sprite({
            texture: trimedTexture,
            width: 40,
            height: 30,
            anchor: 0.5,
            x: 64,
            y: 64,
        });

        rect.setMask({
            mask,
            inverse: true,
        });

        scene.addChild(rect);
        scene.addChild(mask);
    },
};
Repository: pixijs/pixijs
File: tests/visual/scenes/mask/trimed-sprite-mask.scene.ts (TypeScript)

Code before:
export const scene: TestScene = {
    it: 'should correct render trimed alpha mask correctly',
    create: async (scene: Container) =>
    {
        const rect = new Graphics()
            .rect(0, 0, 100, 100)
            .fill(new Color('red'));

        rect.width = 100;
        rect.height = 100;

        const texture = await Assets.load('blurredCircle.png') as Texture;
        const trimedTexture = new Texture({
            source: texture.source,
            frame: new Rectangle(0, 0, 200, 200),
            orig: new Rectangle(0, 0, 400, 300),
            trim: new Rectangle(100, 100, 200, 200)
        });
        const mask = new Sprite(trimedTexture);

        mask.width = 40;
        mask.height = 30;

        mask.anchor.set(0.5);

        mask.position.set(128 / 2);

        rect.mask = mask;
        rect.setMask({
            inverse: true,
        });

        scene.addChild(rect);
        scene.addChild(mask);
    },
};


Review comment (suggestion):
Simpler declarative way to express the same thing:
```suggestion
        const mask = new Sprite({
            texture: trimedTexture,
            width: 40,
            height: 30,
            anchor: 0.5,
            x: 64,
            y: 64,
        });
```


Code after:
export const scene: TestScene = {
    it: 'should correct render trimed alpha mask correctly',
    create: async (scene: Container) =>
    {
        const rect = new Graphics()
            .rect(0, 0, 100, 100)
            .fill(new Color('red'));

        rect.width = 100;
        rect.height = 100;

        const texture = await Assets.load('blurredCircle.png') as Texture;
        const trimedTexture = new Texture({
            source: texture.source,
            frame: new Rectangle(0, 0, 200, 200),
            orig: new Rectangle(0, 0, 400, 300),
            trim: new Rectangle(100, 100, 200, 200)
        });
        const mask = new Sprite({
            texture: trimedTexture,
            width: 40,
            height: 30,
            anchor: 0.5,
            x: 64,
            y: 64,
        });

        rect.setMask({
            mask,
            inverse: true,
        });

        scene.addChild(rect);
        scene.addChild(mask);
    },
};
Repository: pixijs/pixijs
File: src/scene/container/__tests__/getGlobalTransform.test.ts (TypeScript)

Code before:
import { Container } from '../Container';
import { Matrix } from '~/maths';

describe('getGlobalTransform', () =>
{
    let outputMatrix: Matrix;

    beforeEach(() =>
    {
        outputMatrix = new Matrix();
    });

    describe('with no arguments', () =>
    {
        it('should return a new Matrix instance', () =>
        {
            const container = new Container();

            container.x = 100;
            container.y = 100;

            const result = container.getGlobalTransform();

            expect(result).not.toBe(outputMatrix);
            expect(result.tx).toBe(100);
            expect(result.ty).toBe(100);
        });

        it('should return a new Matrix instance even if the first arugmnet is undefined', () =>
        {
            const container = new Container();

            container.x = 100;
            container.y = 100;

            const result = container.getGlobalTransform(undefined);

            expect(result).not.toBe(outputMatrix);
            expect(result.tx).toBe(100);
            expect(result.ty).toBe(100);
        });
    });

    describe('with skipUpdate = false', () =>
    {

Review comment (suggestion):
```suggestion
            const container = new Container({ x: 100, y: 100 });
```


Code after:
import { Container } from '../Container';
import { Matrix } from '~/maths';

describe('getGlobalTransform', () =>
{
    let outputMatrix: Matrix;

    beforeEach(() =>
    {
        outputMatrix = new Matrix();
    });

    describe('with no arguments', () =>
    {
        it('should return a new Matrix instance', () =>
        {
            const container = new Container({ x: 100, y: 100 });

            const result = container.getGlobalTransform();

            expect(result).not.toBe(outputMatrix);
            expect(result.tx).toBe(100);
            expect(result.ty).toBe(100);
        });

        it('should return a new Matrix instance even if the first arugmnet is undefined', () =>
        {
            const container = new Container({ x: 100, y: 100 });

            const result = container.getGlobalTransform(undefined);

            expect(result).not.toBe(outputMatrix);
            expect(result.tx).toBe(100);
            expect(result.ty).toBe(100);
        });
    });

    describe('with skipUpdate = false', () =>
    {
        it('should return local transform when no parent exists', () =>
        {
            const container = new Container({ x: 100, y: 100 });

            const result = container.getGlobalTransform(outputMatrix, false);
Repository: pixijs/pixijs
File: src/scene/container/__tests__/getGlobalTransform.test.ts (TypeScript)

Code before:
    {
        outputMatrix = new Matrix();
    });

    describe('with no arguments', () =>
    {
        it('should return a new Matrix instance', () =>
        {
            const container = new Container();

            container.x = 100;
            container.y = 100;

            const result = container.getGlobalTransform();

            expect(result).not.toBe(outputMatrix);
            expect(result.tx).toBe(100);
            expect(result.ty).toBe(100);
        });

        it('should return a new Matrix instance even if the first arugmnet is undefined', () =>
        {
            const container = new Container();

            container.x = 100;
            container.y = 100;

            const result = container.getGlobalTransform(undefined);

            expect(result).not.toBe(outputMatrix);
            expect(result.tx).toBe(100);
            expect(result.ty).toBe(100);
        });
    });

    describe('with skipUpdate = false', () =>
    {
        it('should return local transform when no parent exists', () =>
        {
            const container = new Container();

            container.x = 100;
            container.y = 100;

            const result = container.getGlobalTransform(outputMatrix, false);

            expect(result).toBe(outputMatrix);
            expect(result.tx).toBe(100);
            expect(result.ty).toBe(100);
        });


Review comment (suggestion):
```suggestion
            const container = new Container({ x: 100, y: 100 });
```


Code after:
    {
        outputMatrix = new Matrix();
    });

    describe('with no arguments', () =>
    {
        it('should return a new Matrix instance', () =>
        {
            const container = new Container({ x: 100, y: 100 });

            const result = container.getGlobalTransform();

            expect(result).not.toBe(outputMatrix);
            expect(result.tx).toBe(100);
            expect(result.ty).toBe(100);
        });

        it('should return a new Matrix instance even if the first arugmnet is undefined', () =>
        {
            const container = new Container({ x: 100, y: 100 });

            const result = container.getGlobalTransform(undefined);

            expect(result).not.toBe(outputMatrix);
            expect(result.tx).toBe(100);
            expect(result.ty).toBe(100);
        });
    });

    describe('with skipUpdate = false', () =>
    {
        it('should return local transform when no parent exists', () =>
        {
            const container = new Container({ x: 100, y: 100 });

            const result = container.getGlobalTransform(outputMatrix, false);

            expect(result).toBe(outputMatrix);
            expect(result.tx).toBe(100);
            expect(result.ty).toBe(100);
        });

        it('should combine transforms with single parent', () =>
        {
            const parent = new Container({ x: 100 });
            const container = new Container({ x: 50 });

            parent.addChild(container);

            const result = container.getGlobalTransform(outputMatrix, false);