Skip to content

Refactor/ast stmt#29

Merged
broccolingual merged 10 commits into
mainfrom
refactor/ast-stmt
Jan 12, 2026
Merged

Refactor/ast stmt#29
broccolingual merged 10 commits into
mainfrom
refactor/ast-stmt

Conversation

@broccolingual
Copy link
Copy Markdown
Owner

No description provided.

@gemini-code-assist
Copy link
Copy Markdown

Summary of Changes

Hello @broccolingual, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

このプルリクエストは、特に文と式の抽象構文木(AST)解析ロジックをリファクタリングします。ループスコープを管理するための新しいRAIIヘルパーを導入し、適切なクリーンアップを保証するとともに、いくつかの主要な関数における条件付き解析ロジックを合理化し、可読性と保守性を向上させています。

Highlights

  • RAIIヘルパーの導入: ループスコープを管理するためのRAIIヘルパー関数with_loop_scopesrc/ast.rsに追加されました。これにより、ループ処理中にエラーが発生した場合でも、pop_loopが確実に呼び出されるようになります。
  • ループ文の解析ロジックのリファクタリング: src/ast/stmt.rs内のiteration_stmt関数が、新しく導入されたwith_loop_scopeヘルパーを使用するようにリファクタリングされました。これにより、whiledo-whileforループのスコープ管理がより堅牢になります。
  • 条件付き解析ロジックの改善: src/ast/expr.rsprimary_expr関数とsrc/ast/stmt.rsstmt関数において、複数のif文と早期リターンを使用していた箇所がif-else if構造に整理され、コードの可読性と構造が向上しました。

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

このプルリクエストは、主にASTの文(statement)の解析ロジックをリファクタリングするものです。while, do-while, for文の解析において、ループスコープの管理をRAIIパターン風のヘルパー関数 with_loop_scope を導入することで、コードの重複を削減し、堅牢性を向上させています。また、primary_exprstmt 関数内の複数の if 文を if-else if チェーンにリファクタリングし、コードの可読性を高めています。

全体として良いリファクタリングですが、新しく導入された with_loop_scope 関数のエラーハンドリングに改善の余地があるため、1点コメントしました。ご確認ください。

Comment thread src/ast.rs Outdated
Comment on lines +272 to +276
self.push_loop(label);
let result = f(self);
// エラーが発生してもpop_loopは必ず実行される
self.pop_loop()?;
result
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

with_loop_scope 関数の現在の実装では、エラーハンドリングに潜在的な問題があります。
クロージャ f がエラーを返し、その後の self.pop_loop() もエラーを返した場合、? 演算子によって f からのエラーが握りつぶされ、pop_loop() のエラーのみが呼び出し元に伝播してしまいます。

f から返されるのはユーザーコードに起因する構文解析エラーである可能性が高い一方、pop_loop() のエラーはコンパイラの内部エラーです。ユーザーにとっては前者のエラー情報の方が重要であるため、それを優先して返すように修正するのが望ましいです。

以下に修正案を提案します。この修正により、f が失敗した場合はそのエラーが常に返されるようになります。

Suggested change
self.push_loop(label);
let result = f(self);
// エラーが発生してもpop_loopは必ず実行される
self.pop_loop()?;
result
self.push_loop(label);
let result = f(self);
// エラーが発生してもpop_loopは必ず実行される
// f()が失敗した場合はそのエラーを優先する
if let Err(pop_err) = self.pop_loop() {
if result.is_ok() {
return Err(pop_err);
}
}
result

@broccolingual
Copy link
Copy Markdown
Owner Author

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

switch文の実装、ありがとうございます。全体的に素晴らしいリファクタリングと機能追加だと感じました。
特に、ループとswitchの両方でbreakを扱えるようにBreakableContextを導入した設計は非常に良いです。また、with_loop_scopeヘルパーを使ってループスコープの管理を安全にしている点も、コードの堅牢性を高める良い改善です。
テストケースも網羅的に追加されており、品質の高いプルリクエストだと思います。
1点だけ、pop_loop関数の堅牢性について改善提案がありますので、ご確認ください。

Comment thread src/ast.rs Outdated
Comment on lines +273 to +276
// breakable_stackからもpop
if let Some(BreakableContext::Loop(_)) = self.breakable_stack.last() {
self.breakable_stack.pop();
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

pop_loop関数内でbreakable_stackから要素をpopする際のロジックに、堅牢性の観点から改善の余地があるかと思います。

現在の実装では、breakable_stackの末尾がLoopコンテキストであるかを確認してからpopしていますが、もしLoopでなかった場合に何もせず処理が継続してしまいます。

if let Some(BreakableContext::Loop(_)) = self.breakable_stack.last() {
    self.breakable_stack.pop();
}

これにより、loop_stackbreakable_stackの状態に不整合が生じ、後続の処理で予期せぬ不具合を引き起こす可能性があります。

popした結果がLoopコンテキストであることをmatchで検証し、そうでなければInternalErrorを返すようにすることで、より堅牢な実装になります。

Suggested change
// breakable_stackからもpop
if let Some(BreakableContext::Loop(_)) = self.breakable_stack.last() {
self.breakable_stack.pop();
}
// breakable_stackからもpop
match self.breakable_stack.pop() {
Some(BreakableContext::Loop(_)) => (),
_ => {
return Err(CompileError::InternalError {
msg: "breakable_stackの整合性が取れていません。Loopコンテキストを期待しました。".to_string(),
});
}
}

@broccolingual
Copy link
Copy Markdown
Owner Author

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

このプルリクエストは、switch, case, default文のサポートを実装するもので、素晴らしい変更だと思います。
主な変更点は以下の通りです。

  • switch文の構文解析とコード生成の実装。
  • break文がループとswitch文の両方で機能するように、BreakableContextを導入してコンテキスト管理を統一。
  • ループ構文の解析にRAIIスタイルのヘルパー with_loop_scope を導入し、コードの堅牢性と可読性を向上。
  • switch文に関する包括的なテストケースの追加。

コードは非常によく設計されており、特にBreakableContextの導入は優れた設計判断です。テストも網羅的で、実装の品質を高く保証しています。

1点だけ、可読性向上のための軽微な修正案を提案しました。全体として、これはマージする価値のある素晴らしいプルリクエストです。

Comment thread src/ast.rs Outdated
Comment on lines +334 to +340
.find_map(|ctx| {
if let BreakableContext::Switch(switch_ctx) = ctx {
Some(switch_ctx.as_mut())
} else {
None
}
})
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

find_map内のクロージャは、if letの代わりにmatch式を使うと、よりRustらしい慣用的な記述になり、可読性が向上します。set_default関数(362-368行目)にも同様の改善が適用できます。

            .find_map(|ctx| match ctx {
                BreakableContext::Switch(switch_ctx) => Some(switch_ctx.as_mut()),
                _ => None,
            })

@broccolingual
Copy link
Copy Markdown
Owner Author

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

switch文のサポート追加、ありがとうございます。構文解析からコード生成、そして網羅的なテストまで、非常に丁寧な実装だと感じました。
特に、ループとswitchのコンテキストをBreakableCtxとして統一し、break文の挙動を正しく扱えるようにした点は素晴らしいです。また、with_loop_scopeヘルパーを導入してループ構文のパーサーをリファクタリングしたことで、コードがクリーンになり、スタック管理の安全性が向上していますね。
いくつか、可読性や一貫性をさらに向上させるための提案をさせていただきます。

Comment thread src/ast.rs Outdated
Comment on lines +270 to +284
self.loop_stack.pop().ok_or_else(|| CompileError::InternalError {
msg: "ループスタックが空です".to_string(),
})?;
// breakable_stackからもpop
match self.breakable_stack.pop() {
Some(BreakableCtx::Loop(_)) => (),
_ => {
return Err(CompileError::InternalError {
msg:
"breakable_stackの整合性が取れていません。Loopコンテキストを期待しました。"
.to_string(),
});
}
}
Ok(())
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

breakable_stackからのpop処理について、match式が直接Resultを返すようにリファクタリングすると、よりRustらしいコードになり可読性が向上します。現在の実装ではmatchのアーム内でreturnしており、関数の出口が複数になっているため、少し追いにくくなっています。

        self.loop_stack.pop().ok_or_else(|| CompileError::InternalError {
            msg: "ループスタックが空です".to_string(),
        })?;
        // breakable_stackからもpop
        match self.breakable_stack.pop() {
            Some(BreakableCtx::Loop(_)) => Ok(()),
            _ => Err(CompileError::InternalError {
                msg: "breakable_stackの整合性が取れていません。Loopコンテキストを期待しました。"
                    .to_string(),
            }),
        }

Comment thread src/ast.rs Outdated
Comment on lines +299 to +304
if let Err(pop_err) = self.pop_loop()
&& result.is_ok()
{
return Err(pop_err);
}
result
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

このRAIIヘルパーは素晴らしいアイデアですね。エラーハンドリング部分について、より明示的な書き方にすると、意図が分かりやすくなるかもしれません。&& result.is_ok()という条件は少し直感的ではない可能性があります。
resultpop_resultをそれぞれ評価し、fの実行でエラーが発生した場合はそのエラーを優先して返す、というロジックをmatch式で表現すると、可読性が向上します。

        let pop_result = self.pop_loop();
        match result {
            Ok(value) => pop_result.map(|_| value),
            Err(e) => Err(e),
        }

Comment thread src/ast.rs Outdated
Comment on lines +359 to +365
.find_map(|ctx| {
if let BreakableCtx::Switch(switch_ctx) = ctx {
Some(switch_ctx.as_mut())
} else {
None
}
})
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

このfind_map内のクロージャは、add_case関数で使われている形式に合わせることで、より簡潔に書くことができます。if let文よりもmatch式を使うことで、コードの一貫性が高まります。

            .find_map(|ctx| match ctx {
                BreakableCtx::Switch(switch_ctx) => Some(switch_ctx.as_mut()),
                _ => None,
            })

@broccolingual
Copy link
Copy Markdown
Owner Author

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

switch文のサポート追加、ありがとうございます。構文解析、break文のためのコンテキスト管理、コード生成、そして網羅的なテストまで、非常に丁寧な実装だと感じました。特に、ループ構文の解析をwith_loop_scopeヘルパーでリファクタリングし、コードの安全性を高めている点は素晴らしいです。

全体として高品質な変更ですが、コードをさらに改善するための提案を2点ほどコメントしました。

  1. Ast構造体内のloop_stackbreakable_stackと機能的に重複しているため、これを削除してコードをシンプルにすること。
  2. add_caseset_default関数内の重複ロジックをヘルパー関数にまとめること。

ご確認いただければ幸いです。

Comment thread src/ast.rs Outdated
Comment on lines +35 to +36
loop_stack: Vec<usize>,
breakable_stack: Vec<BreakableCtx>, // ループとswitchを統合
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Ast構造体にloop_stackbreakable_stackの両方が存在しており、機能が重複しています。breakable_stackはループとswitchの両方のコンテキストを管理できるため、loop_stackは不要です。loop_stackを削除し、breakable_stackのみを使用するようにリファクタリングすることで、コードがよりシンプルになり、整合性も保ちやすくなります。
current_loop_label関数はbreakable_stackを逆順に探索して、最も内側のBreakableCtx::Loopを見つけるように変更することで、loop_stackなしで実装できます。

提案:

  1. Ast構造体からloop_stackフィールドを削除します。
  2. push_looppop_loopbreakable_stackのみを操作するように修正します。
  3. current_loop_labelbreakable_stackを使って再実装します。
// current_loop_labelの実装例
fn current_loop_label(&self) -> Option<usize> {
    self.breakable_stack.iter().rev().find_map(|ctx| {
        if let BreakableCtx::Loop(label) = ctx {
            Some(*label)
        } else {
            None
        }
    })
}

Comment thread src/ast.rs Outdated
Comment on lines +324 to +371
fn add_case(&mut self, val: i64, case_label: usize, span: Span) -> Result<(), CompileError> {
// breakable_stackから最後のswitchを見つける
let switch_ctx = self
.breakable_stack
.iter_mut()
.rev()
.find_map(|ctx| match ctx {
BreakableCtx::Switch(switch_ctx) => Some(switch_ctx.as_mut()),
_ => None,
})
.ok_or_else(|| CompileError::InvalidStmt {
msg: "case文がswitch文の外にあります".to_string(),
span,
})?;
// 重複チェック
if switch_ctx.cases.iter().any(|(v, _)| *v == val) {
return Err(CompileError::InvalidStmt {
msg: format!("case値 {} が重複しています", val),
span,
});
}
switch_ctx.cases.push((val, case_label));
Ok(())
}

fn set_default(&mut self, default_label: usize, span: Span) -> Result<(), CompileError> {
// breakable_stackから最後のswitchを見つける
let switch_ctx = self
.breakable_stack
.iter_mut()
.rev()
.find_map(|ctx| match ctx {
BreakableCtx::Switch(switch_ctx) => Some(switch_ctx.as_mut()),
_ => None,
})
.ok_or_else(|| CompileError::InvalidStmt {
msg: "default文がswitch文の外にあります".to_string(),
span,
})?;
if switch_ctx.default_label.is_some() {
return Err(CompileError::InvalidStmt {
msg: "defaultラベルが重複しています".to_string(),
span,
});
}
switch_ctx.default_label = Some(default_label);
Ok(())
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

add_case関数とset_default関数には、breakable_stackから現在のswitchコンテキストを検索するロジックが重複しています。このロジックを共通のヘルパー関数に切り出すことで、コードの重複をなくし、可読性と保守性を向上させることができます。

提案:
以下のようなヘルパー関数を導入し、add_caseset_defaultから呼び出すように変更してはいかがでしょうか。

fn current_switch_ctx_mut(&mut self, error_msg: &str, span: Span) -> Result<&mut SwitchCtx, CompileError> {
    self.breakable_stack
        .iter_mut()
        .rev()
        .find_map(|ctx| match ctx {
            BreakableCtx::Switch(switch_ctx) => Some(switch_ctx.as_mut()),
            _ => None,
        })
        .ok_or_else(|| CompileError::InvalidStmt {
            msg: error_msg.to_string(),
            span,
        })
}

これにより、add_caseset_defaultの実装は以下のようになります。

// add_case内での呼び出し例
fn add_case(&mut self, val: i64, case_label: usize, span: Span) -> Result<(), CompileError> {
    let switch_ctx = self.current_switch_ctx_mut("case文がswitch文の外にあります", span)?;
    // ... (以降のロジックは同じ)
}

// set_default内での呼び出し例
fn set_default(&mut self, default_label: usize, span: Span) -> Result<(), CompileError> {
    let switch_ctx = self.current_switch_ctx_mut("default文がswitch文の外にあります", span)?;
    // ... (以降のロジックは同じ)
}

@broccolingual
Copy link
Copy Markdown
Owner Author

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

このプルリクエストは、switch, case, default文のサポートを実装するもので、素晴らしい出来栄えです。構文解析、AST、コード生成の各段階で必要な変更が網羅的に行われています。特に、ループとswitch文の両方でbreakを扱えるようにbreakable_stackを導入した点は、非常にクリーンな設計だと感じました。また、with_loop_scopeヘルパーによるRAII的なスコープ管理の導入は、コードの安全性と可読性を高める良いリファクタリングです。追加されたテストケースも非常に包括的で、新機能の品質を高く保証するものとなっています。

1点だけ、with_loop_scope関数のエラーハンドリングに改善の余地があると考え、コメントさせていただきました。全体として、非常によく練られた素晴らしい変更だと思います。

Comment thread src/ast.rs
Comment on lines +297 to +300
match result {
Ok(value) => pop_result.map(|_| value),
Err(e) => Err(e),
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

with_loop_scope関数のエラーハンドリングについてです。
現在の実装では、クロージャf(self)がエラー(resultErr)を返した場合、後続のpop_loop()が返すエラー(pop_resultErr)は無視されてしまいます。

pop_loopInternalErrorを返す可能性があるため、このエラーが無視されるとコンパイラのバグが隠蔽されてしまう可能性があります。

クロージャfからのエラーを優先するのは妥当な判断ですが、fが成功した場合にはpop_loopのエラーもハンドリングすべきです。以下に修正案を提案します。

Suggested change
match result {
Ok(value) => pop_result.map(|_| value),
Err(e) => Err(e),
}
let value = match result {
Ok(v) => v,
Err(e) => return Err(e),
};
// f(self)が成功した場合、pop_loopの結果をチェックする
pop_result?;
Ok(value)

@broccolingual
Copy link
Copy Markdown
Owner Author

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

このプルリクエストは、switch, case, default文のサポートを実装するもので、素晴らしい変更だと思います。
ループとswitchのコンテキストをBreakableCtxで統一し、break文が両方で機能するようにしたリファクタリングは非常に良い設計です。特に、with_loop_scopeというRAIIヘルパーを導入したことで、繰り返し文のパーサーがクリーンになり、スタック操作の安全性が向上しています。
また、switch文に関するテストケースが非常に包括的であり、実装の品質を高く保証している点も評価できます。
いくつかの軽微な改善点をコメントしましたが、全体として非常によくできたプルリクエストです。

Comment thread src/ast.rs
Comment on lines 266 to 275
fn pop_loop(&mut self) -> Result<(), CompileError> {
self.loop_stack
.pop()
.ok_or_else(|| CompileError::InternalError {
msg: "ループスタックが空です".to_string(),
})
.map(|_| ())
match self.breakable_stack.pop() {
Some(BreakableCtx::Loop(_)) => Ok(()),
_ => Err(CompileError::InternalError {
msg: "breakable_stackの整合性が取れていません。Loopコンテキストを期待しました。"
.to_string(),
}),
}?;
Ok(())
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

pop_loop関数の実装が少し冗長に見えます。
match式がすでにResultを返しているため、末尾の?; Ok(())は不要です。match式をそのまま返すように修正することで、コードがよりシンプルになります。

    fn pop_loop(&mut self) -> Result<(), CompileError> {
        match self.breakable_stack.pop() {
            Some(BreakableCtx::Loop(_)) => Ok(()),
            _ => Err(CompileError::InternalError {
                msg: "breakable_stackの整合性が取れていません。Loopコンテキストを期待しました。"
                    .to_string(),
            }),
        }
    }

Comment thread src/ast.rs
Comment on lines +304 to +311
fn pop_switch(&mut self) -> Result<SwitchCtx, CompileError> {
// breakable_stackからpop
if let Some(BreakableCtx::Switch(ctx)) = self.breakable_stack.pop() {
Ok(*ctx)
} else {
Err(CompileError::InternalError { msg: "switchスタックが空です".to_string() })
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

pop_switch関数のエラーハンドリングをより詳細にすることで、デバッグが容易になるかと思います。
現在の実装では、スタックが空の場合と、期待しないコンテキスト(Loop)が見つかった場合の両方で「switchスタックが空です」という同じエラーメッセージが返されます。
match式を使ってそれぞれのケースで異なるエラーメッセージを返すようにすると、内部エラーが発生した際の追跡がしやすくなります。

    fn pop_switch(&mut self) -> Result<SwitchCtx, CompileError> {
        match self.breakable_stack.pop() {
            Some(BreakableCtx::Switch(ctx)) => Ok(*ctx),
            Some(BreakableCtx::Loop(_)) => Err(CompileError::InternalError {
                msg: "breakable_stackの整合性が取れていません。Switchコンテキストを期待しましたが、Loopコンテキストが見つかりました。"
                    .to_string(),
            }),
            None => Err(CompileError::InternalError {
                msg: "switchスタックが空です".to_string(),
            }),
        }
    }

@broccolingual
Copy link
Copy Markdown
Owner Author

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

switch文のサポートを追加する素晴らしいリファクタリングですね。ループとswitchのコンテキストをBreakableCtxで統一する設計は、break文の扱いを綺麗にしており、非常に良いアプローチだと感じました。また、with_loop_scopeヘルパーを導入してループ構文の解析の堅牢性を高めている点も素晴らしいです。

レビューでは、2点ほどフィードバックをさせていただきました。

  1. switch文の解析において、エラー発生時にpop_switchが呼ばれず、パーサーの状態が不正になる可能性がある重大な問題を指摘しました。
  2. switch文のコード生成について、将来的なパフォーマンス向上のための改善案を提案しました。

テストケースが非常に網羅的で、switch文の様々な側面をカバーしている点も大変素晴らしいです。
ご確認のほど、よろしくお願いいたします。

Comment thread src/ast/stmt.rs
Comment on lines +92 to +116
if let Some(span) = self.consume_keyword("switch") {
let label = self.next_label();
self.expect_punct("(")?;
let cond = self.expr()?.ok_or_else(|| CompileError::InvalidStmt {
msg: "switch文の条件式がありません".to_string(),
span,
})?;
self.expect_punct(")")?;
self.push_switch(label); // switch文のコンテキストをプッシュ
let body = self.stmt()?.ok_or_else(|| CompileError::InvalidStmt {
msg: "switch文の本体がありません".to_string(),
span,
})?;
let switch_ctx = self.pop_switch()?; // switch文のコンテキストをポップして、case情報を取得
return Ok(Some(Box::new(Node::new(
NodeKind::Switch {
cond,
body,
label,
cases: switch_ctx.cases,
default_label: switch_ctx.default_label,
},
span,
))));
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

switch文の解析において、エラー発生時にpush_switchpop_switchの呼び出しがアンバランスになる可能性があり、パーサーの状態が破壊される危険があります。

具体的には、101行目のself.stmt()?がエラーを返した場合、?演算子によってselection_stmt関数が早期リターンするため、105行目のself.pop_switch()が呼び出されません。これにより、breakable_stackSwitchコンテキストが残ってしまい、後続の解析に影響を与えます。

この問題は、ループ構文で導入されたwith_loop_scopeのようなRAIIスタイルのヘルパー関数をswitch文にも導入することで解決できます。push_switchpop_switchの呼び出しをヘルパー関数内にカプセル化することで、エラー発生時でもスタックが正しくクリーンアップされることを保証できます。

src/ast.rswith_switch_scope関数を追加し、それを利用するようにリファクタリングすることをお勧めします。

Comment thread src/x86/stmt.rs
Comment on lines +73 to +92
NodeKind::Switch { cond, body, label, cases, default_label } => {
self.gen_expr(cond)?; // 条件式を評価
self.builder.add_row("pop rax", true);

// 各caseと比較
for (val, case_label) in cases {
self.builder.add_row(&format!("cmp rax, {}", val), true);
self.builder.add_row(&format!("je .L.case.{}", case_label), true);
}

// いずれのcaseにも一致しない場合
if let Some(default_label) = default_label {
self.builder.add_row(&format!("jmp .L.default.{}", default_label), true);
} else {
// defaultがない場合はswitch文を抜ける
self.builder.add_row(&format!("jmp .L.break.{}", label), true);
}
self.gen_stmt(body)?; // switch文の本体
self.builder.add_row(&format!(".L.break.{}:", label), false);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

switch文のコード生成が、caseラベルを線形に検索する方法で実装されています。これは機能的には正しいですが、caseの数が多い場合にパフォーマンスが低下する可能性があります。

多くのコンパイラは、caseの密度や数に応じて、より効率的なコードを生成します。将来的なパフォーマンス向上のために、以下のような高度な手法の導入を検討する価値があります。

  • ジャンプテーブル: caseの値が密に分布している場合(例: 1, 2, 3, 4...)、値から直接ジャンプ先アドレスを計算するテーブルを生成することで、O(1)での分岐が可能になります。
  • 二分探索: caseの値が疎に分布しているが数が多い場合、値を二分探索することで、O(log n)での分岐が可能になります。

現在の実装はシンプルで分かりやすいですが、この点を考慮しておくと、将来的にコンパイラの性能をさらに向上させることができます。

@broccolingual broccolingual merged commit 47e38ce into main Jan 12, 2026
1 check passed
@broccolingual broccolingual deleted the refactor/ast-stmt branch January 12, 2026 02:16
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.

1 participant