Refactor/ast expr#28
Conversation
Summary of ChangesHello @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! This pull request refactors the Abstract Syntax Tree (AST) expression parsing logic by introducing generic helper functions for binary, unary, and logical operators. This significantly cleans up and modularizes the expression parsing code. Additionally, it extends the compiler's capabilities by adding support for the comma operator, unary plus and minus, and the greater than/equal relational operators, along with corresponding type checking and x86 code generation. Highlights
🧠 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 AssistThe 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
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| loop { | ||
| let mut matched = false; | ||
| for (op_str, constructor) in operators { | ||
| if let Some(span) = self.consume_punct(op_str) { | ||
| let lhs = node.ok_or_else(|| CompileError::InvalidExpr { | ||
| msg: format!("'{}'演算子の左辺値がありません", op_str), | ||
| span, | ||
| })?; | ||
| let rhs = next_parser(self)?.ok_or_else(|| CompileError::InvalidExpr { | ||
| msg: format!("'{}'演算子の右辺値がありません", op_str), | ||
| span, | ||
| })?; | ||
| node = Some(Box::new(constructor(lhs, rhs, span)?)); | ||
| matched = true; | ||
| break; | ||
| } | ||
| } | ||
| if !matched { | ||
| return Ok(node); | ||
| } | ||
| } |
There was a problem hiding this comment.
matched フラグを使ったループは、よりRustらしいイディオムで書き換えることで、さらに可読性を高めることができます。peek_punct で演算子を先に確認し、iter().find() を使うことで、ネストを減らし、ループの意図をより明確にできます。
loop {
if let Some((op_str, constructor)) = operators.iter().find(|(s, _)| self.peek_punct(s)) {
let span = self.consume_punct(op_str).unwrap(); // peek_punctで確認済みのため安全
let lhs = node.ok_or_else(|| CompileError::InvalidExpr {
msg: format!("'{}'演算子の左辺値がありません", op_str),
span,
})?;
let rhs = next_parser(self)?.ok_or_else(|| CompileError::InvalidExpr {
msg: format!("'{}'演算子の右辺値がありません", op_str),
span,
})?;
node = Some(Box::new(constructor(lhs, rhs, span)?));
} else {
return Ok(node);
}
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
このプルリクエストは、式の解析ロジックを大幅にリファクタリングするもので、素晴らしい改善です。parse_binary_op、parse_unary_op、parse_logical_opといったヘルパー関数を導入することで、コードの重複が大幅に削減され、各演算子の優先順位と結合規則の処理が非常に明確になりました。
また、カンマ演算子や単項の+/-演算子のサポートが追加され、関連するテストも整備されている点を高く評価します。
1点、単項演算子の型チェックに関して、浮動小数点型が考慮されていない問題を見つけましたので、修正提案をコメントしました。
全体として、コードの品質と保守性を大きく向上させる優れたリファクタリングです。
| if expr_ty.is_integer() { | ||
| TypeRef::register(TypeKind::Int, TypeAttr::default(), None) // 整数拡張 | ||
| } else { | ||
| return Err(CompileError::InvalidExpr { | ||
| msg: format!( | ||
| "単項演算子 '{:?}' は整数型にのみ適用可能です: {:?}", | ||
| op, expr_ty | ||
| ), | ||
| span, | ||
| }); | ||
| } |
There was a problem hiding this comment.
C言語の仕様では、単項の+および-演算子は整数型だけでなく、浮動小数点型にも適用できます。現在の実装では整数型のみを許可しているため、floatやdouble型の変数に対してこれらの演算子を使うとコンパイルエラーになってしまいます。
算術型（整数と浮動小数点）を許可するように修正し、エラーメッセージも修正することを提案します。
| if expr_ty.is_integer() { | |
| TypeRef::register(TypeKind::Int, TypeAttr::default(), None) // 整数拡張 | |
| } else { | |
| return Err(CompileError::InvalidExpr { | |
| msg: format!( | |
| "単項演算子 '{:?}' は整数型にのみ適用可能です: {:?}", | |
| op, expr_ty | |
| ), | |
| span, | |
| }); | |
| } | |
| if expr_ty.is_integer() { | |
| TypeRef::register(TypeKind::Int, TypeAttr::default(), None) // 整数拡張 | |
| } else if expr_ty.is_floating_point() { | |
| expr_ty // 浮動小数点型はそのまま | |
| } else { | |
| return Err(CompileError::InvalidExpr { | |
| msg: format!( | |
| "単項演算子 '{:?}' は算術型にのみ適用可能です: {:?}", | |
| op, expr_ty | |
| ), | |
| span, | |
| }); | |
| } |
| loop { | ||
| if let Some((op_str, constructor)) = operators.iter().find(|(s, _)| self.peek_punct(s)) | ||
| { | ||
| let span = self.consume_punct(op_str).unwrap(); // peek_punctで確認済みのため安全 |
There was a problem hiding this comment.
peek_punctで存在を確認しているため、ここのunwrap()がパニックすることはないはずですが、より安全で意図が明確になるようにexpect()を使うことをお勧めします。expect()を使うと、万が一ロジックの変更などでパニックした場合に、デバッグに役立つメッセージを残すことができます。
| let span = self.consume_punct(op_str).unwrap(); // peek_punctで確認済みのため安全 | |
| let span = self.consume_punct(op_str).expect("peek_punctで確認済みのため安全なはず"); // peek_punctで確認済みのため安全 |
1 participant
No description provided.