Skip to content

Conversation

@iwoplaza
Copy link
Collaborator

No description provided.

@github-actions
Copy link

pkg.pr.new

packages
Ready to be installed by your favorite package manager ⬇️

https://pkg.pr.new/software-mansion/TypeGPU/typegpu@70d51dc33ba3c0f40e8ba00e9759c5fb58ad64cf
https://pkg.pr.new/software-mansion/TypeGPU/@typegpu/noise@70d51dc33ba3c0f40e8ba00e9759c5fb58ad64cf
https://pkg.pr.new/software-mansion/TypeGPU/unplugin-typegpu@70d51dc33ba3c0f40e8ba00e9759c5fb58ad64cf

benchmark
view benchmark

commit
view commit

@iwoplaza iwoplaza merged commit 588b090 into main Dec 22, 2025
3 checks passed
@iwoplaza iwoplaza deleted the fix/params-external-name-clash branch December 22, 2025 19:28
@aleksanderkatan aleksanderkatan restored the fix/params-external-name-clash branch December 23, 2025 09:16
Comment on lines +249 to +252
return this
.#usedFunctionScopeNamesStack[
this.#usedFunctionScopeNamesStack.length - 1
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
return this
.#usedFunctionScopeNamesStack[
this.#usedFunctionScopeNamesStack.length - 1
];
return this.#usedFunctionScopeNamesStack.at(-1);

Copy link
Collaborator

@cieplypolar cieplypolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 🎄

if (global) {
this.#usedNames.add(name);
} else {
this.usedFunctionScopeNames?.add(name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the fact that the function scope stack can be empty. I’d prefer to add an invariant (it is never empty) or a comment explaining why this state is possible

makeValid(primer: string): string {
if (isValidIdentifier(primer)) {
if (isValidIdentifier(primer) && !this.#usedNames.has(primer)) {
this.usedFunctionScopeNames?.add(primer);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants