Refactor/ast decl#27
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 undertakes a significant refactoring of the declaration parsing components within the compiler's Abstract Syntax Tree (AST) module. The primary goal is to enhance the clarity, maintainability, and correctness of how the compiler interprets C-style declarations, particularly those involving complex types like function pointers and arrays. By modularizing parsing logic and improving the handling of type qualifiers and parenthesized constructs, the changes contribute to a more robust and accurate front-end for the compiler. 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}
There was a problem hiding this comment.
Code Review
このプルリクエストは、宣言のパース処理、特に複雑な宣言子（関数ポインタなど）の解釈を改善するための大規模なリファクタリングです。主な変更点は以下の通りです。
external_declから関数定義のパースロジックをfunc_def関数に抽出し、見通しを良くしました。ptr関数の実装を再帰からループに変更し、型修飾子 (constなど) を正しく扱えるように修正しました。direct_declaratorをリファクタリングし、括弧を含む複雑な宣言子（例:int (*p)()）を正しくパースできるように、プレースホルダー型を用いた手法に変更しました。replace_typeヘルパーをTypeRefに追加し、上記のプレースホルダー置換を実現しました。- 変数名をより具体的に（例:
specs->decl_specs）変更し、コードの可読性を向上させました。 - 関数ポインタに関する新しいテストケースを追加し、リファクタリングの正当性を担保しています。
全体として、コードの正確性と保守性を大幅に向上させる素晴らしいリファクタリングです。1点、抽象宣言子のパース処理に declarator と同様の修正が適用されておらず、バグが残っている可能性があるため、修正を提案します。
| fn direct_abst_declarator(&mut self, base_ty: &TypeRef) -> Result<TypeRef, CompileError> { | ||
| let mut current_ty = if self.consume_punct("(").is_some() { | ||
| let current_ty = if self.consume_punct("(").is_some() { | ||
| let inner_ty = self.abst_declarator(base_ty)?; | ||
| self.expect_punct(")")?; | ||
| inner_ty | ||
| } else { | ||
| *base_ty | ||
| }; | ||
| current_ty = self.parse_abst_postfix_declarators(¤t_ty)?; | ||
| Ok(current_ty) | ||
| self.direct_abst_declarator_suffix(¤t_ty) | ||
| } |
There was a problem hiding this comment.
抽象宣言子 direct_abst_declarator のパース処理に、通常の宣言子 direct_declarator で行われたようなプレースホルダーを用いた修正が適用されていないため、括弧でグループ化された抽象宣言子（例: int (*)[5]）を正しく解釈できません。
現在の実装では、(D)S という構造（Dが内側の宣言子、Sが後置修飾子）を (DS) のように誤って結合してしまいます。例えば sizeof(int (*)[5]) のようなキャスト式で int *[5]（intへのポインタの配列）と誤解釈されるバグに繋がります。
direct_declarator の修正（600-620行目）と同様に、プレースホルダーを使って型を正しく合成するロジックに修正すべきです。
| fn direct_abst_declarator(&mut self, base_ty: &TypeRef) -> Result<TypeRef, CompileError> { | |
| let mut current_ty = if self.consume_punct("(").is_some() { | |
| let current_ty = if self.consume_punct("(").is_some() { | |
| let inner_ty = self.abst_declarator(base_ty)?; | |
| self.expect_punct(")")?; | |
| inner_ty | |
| } else { | |
| *base_ty | |
| }; | |
| current_ty = self.parse_abst_postfix_declarators(¤t_ty)?; | |
| Ok(current_ty) | |
| self.direct_abst_declarator_suffix(¤t_ty) | |
| } | |
| fn direct_abst_declarator(&mut self, base_ty: &TypeRef) -> Result<TypeRef, CompileError> { | |
| if self.consume_punct("(").is_some() { | |
| let placeholder = TypeRef::default(); | |
| let inner_ty = self.abst_declarator(&placeholder)?; | |
| self.expect_punct(")")?; | |
| let suffix_ty = self.direct_abst_declarator_suffix(base_ty)?; | |
| Ok(inner_ty.replace_type(placeholder, suffix_ty)) | |
| } else { | |
| self.direct_abst_declarator_suffix(base_ty) | |
| } | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
宣言子のパースに関する大規模なリファクタリング、お疲れ様です。素晴らしい改善だと感じました。
特に、関数ポインタのような複雑な宣言子を正しく解析するための direct_declarator の再実装は、C言語の厄介な仕様を見事に解決しています。プレースホルダー型と replace_type を使うアプローチは非常にスマートです。
また、これまで無視されていたポインタや配列の型修飾子 (const など) に対応した点も、コンパイラの堅牢性を高める重要な改善です。
全体的にコードのモジュール性が向上し、可読性も高まっています。
いくつか軽微な修正点を提案させていただきました。主に、不要な可変参照の削除や、リファクタリングによって生じたと思われる到達不能コード（デッドコード）の削除に関するものです。これら修正により、コードの保守性がさらに向上するかと思います。
素晴らしいプルリクエストをありがとうございます。
| fn validate_func_prototype(&mut self, decl: &Decl) -> Result<(), CompileError> { | ||
| if let Some(symbol) = self.find_symbol_mut(&decl.name) { |
There was a problem hiding this comment.
この関数はシンボルテーブルを変更しないため、&mut selfとfind_symbol_mutは不要です。借用規則をより厳密にするために、&selfとfind_symbolを使用することを推奨します。
これにより、この関数が状態を変更しないことがシグネチャから明確になります。
| fn validate_func_prototype(&mut self, decl: &Decl) -> Result<(), CompileError> { | |
| if let Some(symbol) = self.find_symbol_mut(&decl.name) { | |
| fn validate_func_prototype(&self, decl: &Decl) -> Result<(), CompileError> { | |
| if let Some(symbol) = self.find_symbol(&decl.name) { |
1 participant
No description provided.