Skip to content

Conversation

@chibash
Copy link
Member

@chibash chibash commented Sep 19, 2025

No description provided.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a Transpiler abstract base class to refactor the existing shell transpilation logic, creating a cleaner separation of concerns and improving code organization.

  • Creates an abstract Transpiler class with common transpilation functionality and module management
  • Refactors the existing Transpiler class in shell.ts to extend the new base class as ShellTranspiler
  • Moves shared transpilation logic (file reading, module caching, session management) into the base class

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
server/src/transpiler/transpiler.ts Introduces the abstract Transpiler base class with common transpilation functionality
server/src/transpiler/shell.ts Refactors the existing transpiler implementation to extend the new base class and removes duplicate code

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.


getBaseGlobalNames() {
if (!this.baseGlobalNames)
throw 'baseGlobalNames is not set'
Copy link

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

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

Use proper Error objects instead of throwing strings. Replace with throw new Error('baseGlobalNames is not set').

Suggested change
throw 'baseGlobalNames is not set'
throw new Error('baseGlobalNames is not set')

Copilot uses AI. Check for mistakes.
Comment on lines +80 to +81
execSync(`cc -DTEST64 -O2 -shared -fPIC -o ${this.cRuntimeSo} ${cRuntimeC} ${cFile}`)
execSync(`cc -DTEST64 -O2 -o ${dir}/shell ${shellC} ${this.cRuntimeSo} -lm -ldl`)
Copy link

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

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

Using this.cRuntimeSo in a static method will cause an error since this is not available in static context. Use ShellTranspiler.cRuntimeSo instead.

Suggested change
execSync(`cc -DTEST64 -O2 -shared -fPIC -o ${this.cRuntimeSo} ${cRuntimeC} ${cFile}`)
execSync(`cc -DTEST64 -O2 -o ${dir}/shell ${shellC} ${this.cRuntimeSo} -lm -ldl`)
execSync(`cc -DTEST64 -O2 -shared -fPIC -o ${ShellTranspiler.cRuntimeSo} ${cRuntimeC} ${cFile}`)
execSync(`cc -DTEST64 -O2 -o ${dir}/shell ${shellC} ${ShellTranspiler.cRuntimeSo} -lm -ldl`)

Copilot uses AI. Check for mistakes.
Comment on lines +80 to +81
execSync(`cc -DTEST64 -O2 -shared -fPIC -o ${this.cRuntimeSo} ${cRuntimeC} ${cFile}`)
execSync(`cc -DTEST64 -O2 -o ${dir}/shell ${shellC} ${this.cRuntimeSo} -lm -ldl`)
Copy link

Copilot AI Sep 19, 2025

Choose a reason for hiding this comment

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

Using this.cRuntimeSo in a static method will cause an error since this is not available in static context. Use ShellTranspiler.cRuntimeSo instead.

Suggested change
execSync(`cc -DTEST64 -O2 -shared -fPIC -o ${this.cRuntimeSo} ${cRuntimeC} ${cFile}`)
execSync(`cc -DTEST64 -O2 -o ${dir}/shell ${shellC} ${this.cRuntimeSo} -lm -ldl`)
execSync(`cc -DTEST64 -O2 -shared -fPIC -o ${ShellTranspiler.cRuntimeSo} ${cRuntimeC} ${cFile}`)
execSync(`cc -DTEST64 -O2 -o ${dir}/shell ${shellC} ${ShellTranspiler.cRuntimeSo} -lm -ldl`)

Copilot uses AI. Check for mistakes.
@maejima-fumika maejima-fumika merged commit 4d26693 into csg-tokyo:main Sep 22, 2025
1 check passed
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.

2 participants