refactor(joinir): Phase 223 cleanup - impl consolidation & comment cleanup
Improves code organization and readability after Phase 223-3 implementation. ## Task R-1: impl Block Consolidation **loop_body_cond_promoter.rs**: - Merged 2 separate impl blocks into single unified impl - Added section headers for better organization: - Public API (extract_continue_condition, try_promote_for_condition) - Private Helpers (contains_continue) - Improved logical flow and discoverability ## Task R-2: Phase Comment Cleanup **pattern4_with_continue.rs**: - Simplified Phase 223-3 comments (removed redundant phase numbers) - Added concise section header explaining LoopBodyLocal promotion - Clarified inline comments for better understanding - Maintained implementation intent while reducing noise ## Impact - No functional changes - All tests PASS (5 cond_promoter tests, 8 pattern4 tests) - Better code organization for future maintenance - Cleaner git history for Phase 223 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
205
docs/development/current/main/phase223-refactoring-tasks.md
Normal file
205
docs/development/current/main/phase223-refactoring-tasks.md
Normal file
@ -0,0 +1,205 @@
|
|||||||
|
# Phase 223 Refactoring Tasks
|
||||||
|
|
||||||
|
Phase 223-3 実装完了後のコード品質改善タスク一覧だよ。
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Task R-1: impl ブロック統合(優先度: 高)
|
||||||
|
|
||||||
|
**問題**:
|
||||||
|
`loop_body_cond_promoter.rs` で同じ struct に対して impl が2箇所に分かれている。
|
||||||
|
|
||||||
|
**現状**:
|
||||||
|
```rust
|
||||||
|
impl LoopBodyCondPromoter {
|
||||||
|
pub fn extract_continue_condition(...) { ... }
|
||||||
|
fn contains_continue(...) { ... }
|
||||||
|
}
|
||||||
|
|
||||||
|
impl LoopBodyCondPromoter {
|
||||||
|
pub fn try_promote_for_condition(...) { ... }
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
**やること**:
|
||||||
|
1. 2つの impl ブロックを1つに統合
|
||||||
|
2. メソッドの順序を論理的に整理:
|
||||||
|
- Public API (extract_continue_condition, try_promote_for_condition)
|
||||||
|
- Private helpers (contains_continue)
|
||||||
|
|
||||||
|
**影響範囲**: `src/mir/loop_pattern_detection/loop_body_cond_promoter.rs` のみ
|
||||||
|
|
||||||
|
**テスト**: `cargo test --release --lib cond_promoter` (5 tests PASS を確認)
|
||||||
|
|
||||||
|
**期待**: コード可読性向上、構造の明確化
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Task R-2: Phase コメント整理(優先度: 中)
|
||||||
|
|
||||||
|
**問題**:
|
||||||
|
`pattern4_with_continue.rs` に古い Phase 番号のコメントが残っている。
|
||||||
|
|
||||||
|
**現状**:
|
||||||
|
```rust
|
||||||
|
// Phase 171-C-3: LoopBodyCarrierPromoter integration (削除済みコードへの参照)
|
||||||
|
// Phase 223-3: LoopBodyCondPromoter integration (新しいコード)
|
||||||
|
```
|
||||||
|
|
||||||
|
**やること**:
|
||||||
|
1. Phase 171-C-3 の古いコメントを削除
|
||||||
|
2. Phase 223-3 のコメントを簡潔に整理
|
||||||
|
3. 実装の意図が明確になるようコメントを調整
|
||||||
|
|
||||||
|
**候補**:
|
||||||
|
```rust
|
||||||
|
// Phase 223-3: LoopBodyLocal Condition Promotion
|
||||||
|
// Check for LoopBodyLocal in loop/continue conditions and attempt promotion.
|
||||||
|
// Safe Trim patterns (Category A-3: skip_whitespace) are promoted to carriers.
|
||||||
|
```
|
||||||
|
|
||||||
|
**影響範囲**: `src/mir/builder/control_flow/joinir/patterns/pattern4_with_continue.rs`
|
||||||
|
|
||||||
|
**テスト**: `cargo test --release --lib pattern4` (8 tests PASS を確認)
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Task R-3: エラーメッセージの定数化(優先度: 低)
|
||||||
|
|
||||||
|
**問題**:
|
||||||
|
Pattern4 のエラーメッセージが複数箇所にハードコードされている。
|
||||||
|
|
||||||
|
**現状**:
|
||||||
|
```rust
|
||||||
|
// pattern4_with_continue.rs
|
||||||
|
return Err(format!(
|
||||||
|
"[cf_loop/pattern4] Cannot promote LoopBodyLocal variables {:?}: {}",
|
||||||
|
vars, reason
|
||||||
|
));
|
||||||
|
|
||||||
|
// loop_body_cond_promoter.rs
|
||||||
|
eprintln!(
|
||||||
|
"[cond_promoter] Cannot promote LoopBodyLocal variables {:?}: {}",
|
||||||
|
vars, reason
|
||||||
|
);
|
||||||
|
```
|
||||||
|
|
||||||
|
**やること**:
|
||||||
|
1. `error_messages.rs` に Pattern4 用のエラーメッセージ関数を追加
|
||||||
|
2. 既存の `format_error_pattern4` パターンに合わせる
|
||||||
|
3. ハードコードされたメッセージを関数呼び出しに置き換え
|
||||||
|
|
||||||
|
**影響範囲**:
|
||||||
|
- `src/mir/loop_pattern_detection/error_messages.rs` (追加)
|
||||||
|
- `src/mir/builder/control_flow/joinir/patterns/pattern4_with_continue.rs` (置き換え)
|
||||||
|
- `src/mir/loop_pattern_detection/loop_body_cond_promoter.rs` (置き換え)
|
||||||
|
|
||||||
|
**テスト**: `cargo test --release --lib error_messages pattern4 cond_promoter`
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Task R-4: Pattern4ConditionAnalyzer Box 化(優先度: 低、将来対応)
|
||||||
|
|
||||||
|
**問題**:
|
||||||
|
Pattern4 の条件分析ロジックが `pattern4_with_continue.rs` に直接書かれている。
|
||||||
|
|
||||||
|
**現状**:
|
||||||
|
```rust
|
||||||
|
// pattern4_with_continue.rs 内にインライン
|
||||||
|
let continue_cond = LoopBodyCondPromoter::extract_continue_condition(body_to_analyze);
|
||||||
|
let conditions_to_analyze: Vec<&ASTNode> = if let Some(cont_cond) = continue_cond {
|
||||||
|
vec![condition, cont_cond]
|
||||||
|
} else {
|
||||||
|
vec![condition]
|
||||||
|
};
|
||||||
|
let cond_scope = LoopConditionScopeBox::analyze(...);
|
||||||
|
```
|
||||||
|
|
||||||
|
**候補**:
|
||||||
|
```rust
|
||||||
|
// 新規 Box: Pattern4ConditionAnalyzer
|
||||||
|
pub struct Pattern4ConditionAnalyzer;
|
||||||
|
|
||||||
|
impl Pattern4ConditionAnalyzer {
|
||||||
|
/// Analyze all conditions (header + continue) for Pattern4
|
||||||
|
pub fn analyze_all_conditions(
|
||||||
|
loop_param_name: &str,
|
||||||
|
header_cond: &ASTNode,
|
||||||
|
body: &[ASTNode],
|
||||||
|
scope: &LoopScopeShape,
|
||||||
|
) -> LoopConditionScope {
|
||||||
|
// Extract continue condition
|
||||||
|
// Combine with header condition
|
||||||
|
// Call LoopConditionScopeBox::analyze
|
||||||
|
}
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
**やること**:
|
||||||
|
1. 新規ファイル `src/mir/builder/control_flow/joinir/patterns/pattern4_condition_analyzer.rs` 作成
|
||||||
|
2. 条件分析ロジックを切り出し
|
||||||
|
3. Pattern4 から呼び出し
|
||||||
|
4. ユニットテスト追加
|
||||||
|
|
||||||
|
**影響範囲**:
|
||||||
|
- 新規: `pattern4_condition_analyzer.rs`
|
||||||
|
- 修正: `pattern4_with_continue.rs`
|
||||||
|
- 修正: `mod.rs` (モジュール登録)
|
||||||
|
|
||||||
|
**テスト**: 新規テスト + 既存 pattern4 テスト
|
||||||
|
|
||||||
|
**Note**: 現状で十分動作しているため、Pattern4 の複雑度が増した時に検討。
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Task R-5: LoopConditionScopeBox API 改善(優先度: 低、将来対応)
|
||||||
|
|
||||||
|
**問題**:
|
||||||
|
`analyze()` が `&[&ASTNode]` を受け取るため、呼び出し側で Vec を作る必要がある。
|
||||||
|
|
||||||
|
**現状**:
|
||||||
|
```rust
|
||||||
|
let conditions_to_analyze: Vec<&ASTNode> = ...;
|
||||||
|
LoopConditionScopeBox::analyze(&loop_var, &conditions_to_analyze, ...);
|
||||||
|
```
|
||||||
|
|
||||||
|
**候補**:
|
||||||
|
```rust
|
||||||
|
// Option 1: Iterator ベース
|
||||||
|
LoopConditionScopeBox::analyze_iter(
|
||||||
|
&loop_var,
|
||||||
|
[condition, cont_cond].iter().filter_map(|&c| c),
|
||||||
|
scope,
|
||||||
|
);
|
||||||
|
|
||||||
|
// Option 2: 複数シグネチャ
|
||||||
|
impl LoopConditionScopeBox {
|
||||||
|
pub fn analyze(...) -> LoopConditionScope { ... }
|
||||||
|
pub fn analyze_single(cond: &ASTNode, ...) -> LoopConditionScope { ... }
|
||||||
|
pub fn analyze_pair(cond1: &ASTNode, cond2: Option<&ASTNode>, ...) -> LoopConditionScope { ... }
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
**影響範囲**: 広範囲(LoopConditionScopeBox を使う全ての箇所)
|
||||||
|
|
||||||
|
**Note**: 影響範囲が大きいため、現状維持。必要になったら検討。
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 実施方針
|
||||||
|
|
||||||
|
1. **Task R-1 (impl 統合)**: 簡単・影響小、すぐ実施可能
|
||||||
|
2. **Task R-2 (コメント整理)**: 簡単・影響小、R-1 と一緒に実施可能
|
||||||
|
3. **Task R-3 (エラーメッセージ)**: 中程度、既存パターンあり、実施可能
|
||||||
|
4. **Task R-4/R-5**: 優先度低、必要になったら検討
|
||||||
|
|
||||||
|
## フィードバック項目
|
||||||
|
|
||||||
|
- [ ] R-1/R-2 は一緒にやる?
|
||||||
|
- [ ] R-3 も含める?
|
||||||
|
- [ ] 他に気になる箇所ある?
|
||||||
|
- [ ] コメントの書き方(日本語 vs 英語、詳細度)の好み
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
**Note**: 全て既存テスト PASS を前提。退行防止のため各タスク後に `cargo test --release` 実行。
|
||||||
@ -209,16 +209,18 @@ impl MirBuilder {
|
|||||||
// Phase 195: Use unified trace
|
// Phase 195: Use unified trace
|
||||||
trace::trace().varmap("pattern4_start", &self.variable_map);
|
trace::trace().varmap("pattern4_start", &self.variable_map);
|
||||||
|
|
||||||
// Phase 223-3: LoopBodyCondPromoter integration
|
// Phase 223-3: LoopBodyLocal Condition Promotion
|
||||||
// Check for LoopBodyLocal in continue condition and attempt promotion
|
//
|
||||||
|
// Check for LoopBodyLocal in loop/continue conditions and attempt promotion.
|
||||||
|
// Safe Trim patterns (Category A-3: skip_whitespace) are promoted to carriers.
|
||||||
{
|
{
|
||||||
use crate::mir::loop_pattern_detection::loop_condition_scope::LoopConditionScopeBox;
|
use crate::mir::loop_pattern_detection::loop_condition_scope::LoopConditionScopeBox;
|
||||||
use crate::mir::loop_pattern_detection::loop_body_cond_promoter::{
|
use crate::mir::loop_pattern_detection::loop_body_cond_promoter::{
|
||||||
LoopBodyCondPromoter, ConditionPromotionRequest, ConditionPromotionResult,
|
LoopBodyCondPromoter, ConditionPromotionRequest, ConditionPromotionResult,
|
||||||
};
|
};
|
||||||
|
|
||||||
// Phase 223-3: Extract continue condition from body (if present)
|
// Extract continue condition from body (if present)
|
||||||
// This handles skip_whitespace pattern: if ch == " " || ... { continue }
|
// Handles skip_whitespace pattern: if ch == " " || ... { continue }
|
||||||
let continue_cond = LoopBodyCondPromoter::extract_continue_condition(body_to_analyze);
|
let continue_cond = LoopBodyCondPromoter::extract_continue_condition(body_to_analyze);
|
||||||
|
|
||||||
// Analyze both header condition and continue condition for LoopBodyLocal
|
// Analyze both header condition and continue condition for LoopBodyLocal
|
||||||
@ -235,7 +237,7 @@ impl MirBuilder {
|
|||||||
);
|
);
|
||||||
|
|
||||||
if cond_scope.has_loop_body_local() {
|
if cond_scope.has_loop_body_local() {
|
||||||
// Phase 223-3: Try promotion using LoopBodyCondPromoter
|
// Try promotion using LoopBodyCondPromoter
|
||||||
let promotion_req = ConditionPromotionRequest {
|
let promotion_req = ConditionPromotionRequest {
|
||||||
loop_param_name: &loop_var_name,
|
loop_param_name: &loop_var_name,
|
||||||
cond_scope: &cond_scope,
|
cond_scope: &cond_scope,
|
||||||
@ -256,7 +258,7 @@ impl MirBuilder {
|
|||||||
promoted_var, carrier_name
|
promoted_var, carrier_name
|
||||||
);
|
);
|
||||||
|
|
||||||
// Phase 223-3: Merge promoted carrier into existing CarrierInfo
|
// Merge promoted carrier into existing CarrierInfo
|
||||||
carrier_info.merge_from(&promoted_carrier);
|
carrier_info.merge_from(&promoted_carrier);
|
||||||
|
|
||||||
eprintln!(
|
eprintln!(
|
||||||
@ -265,7 +267,7 @@ impl MirBuilder {
|
|||||||
carrier_info.carrier_count()
|
carrier_info.carrier_count()
|
||||||
);
|
);
|
||||||
|
|
||||||
// Phase 223-3: Check if this is a safe Trim pattern
|
// Check if this is a safe Trim pattern
|
||||||
if let Some(helper) = carrier_info.trim_helper() {
|
if let Some(helper) = carrier_info.trim_helper() {
|
||||||
if helper.is_safe_trim() {
|
if helper.is_safe_trim() {
|
||||||
eprintln!(
|
eprintln!(
|
||||||
@ -275,8 +277,7 @@ impl MirBuilder {
|
|||||||
"[pattern4/cond_promoter] Carrier: '{}', original var: '{}', whitespace chars: {:?}",
|
"[pattern4/cond_promoter] Carrier: '{}', original var: '{}', whitespace chars: {:?}",
|
||||||
helper.carrier_name, helper.original_var, helper.whitespace_chars
|
helper.carrier_name, helper.original_var, helper.whitespace_chars
|
||||||
);
|
);
|
||||||
// Phase 223-3: Continue with Pattern4 lowering
|
// Continue with Pattern4 lowering (fall through)
|
||||||
// (fall through to normal lowering path)
|
|
||||||
} else {
|
} else {
|
||||||
return Err(format!(
|
return Err(format!(
|
||||||
"[cf_loop/pattern4] Trim pattern detected but not safe: carrier='{}', whitespace_count={}",
|
"[cf_loop/pattern4] Trim pattern detected but not safe: carrier='{}', whitespace_count={}",
|
||||||
@ -285,11 +286,10 @@ impl MirBuilder {
|
|||||||
));
|
));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
// Phase 223-3: Continue with normal Pattern4 lowering
|
// Carrier promoted and merged, proceed with normal lowering
|
||||||
// The carrier has been promoted and merged, lowering can proceed
|
|
||||||
}
|
}
|
||||||
ConditionPromotionResult::CannotPromote { reason, vars } => {
|
ConditionPromotionResult::CannotPromote { reason, vars } => {
|
||||||
// Phase 223-3: Fail-Fast on promotion failure
|
// Fail-Fast on promotion failure
|
||||||
return Err(format!(
|
return Err(format!(
|
||||||
"[cf_loop/pattern4] Cannot promote LoopBodyLocal variables {:?}: {}",
|
"[cf_loop/pattern4] Cannot promote LoopBodyLocal variables {:?}: {}",
|
||||||
vars, reason
|
vars, reason
|
||||||
|
|||||||
@ -85,6 +85,10 @@ pub enum ConditionPromotionResult {
|
|||||||
pub struct LoopBodyCondPromoter;
|
pub struct LoopBodyCondPromoter;
|
||||||
|
|
||||||
impl LoopBodyCondPromoter {
|
impl LoopBodyCondPromoter {
|
||||||
|
// ========================================================================
|
||||||
|
// Public API
|
||||||
|
// ========================================================================
|
||||||
|
|
||||||
/// Extract continue condition from loop body
|
/// Extract continue condition from loop body
|
||||||
///
|
///
|
||||||
/// Finds the first if statement with continue in then-branch and returns its condition.
|
/// Finds the first if statement with continue in then-branch and returns its condition.
|
||||||
@ -123,33 +127,6 @@ impl LoopBodyCondPromoter {
|
|||||||
None
|
None
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Check if statements contain a continue statement
|
|
||||||
fn contains_continue(stmts: &[ASTNode]) -> bool {
|
|
||||||
for stmt in stmts {
|
|
||||||
match stmt {
|
|
||||||
ASTNode::Continue { .. } => return true,
|
|
||||||
ASTNode::If {
|
|
||||||
then_body,
|
|
||||||
else_body,
|
|
||||||
..
|
|
||||||
} => {
|
|
||||||
if Self::contains_continue(then_body) {
|
|
||||||
return true;
|
|
||||||
}
|
|
||||||
if let Some(else_stmts) = else_body {
|
|
||||||
if Self::contains_continue(else_stmts) {
|
|
||||||
return true;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
_ => {}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
false
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
impl LoopBodyCondPromoter {
|
|
||||||
/// Try to promote LoopBodyLocal variables in conditions
|
/// Try to promote LoopBodyLocal variables in conditions
|
||||||
///
|
///
|
||||||
/// ## P0 Requirements (Category A-3)
|
/// ## P0 Requirements (Category A-3)
|
||||||
@ -223,6 +200,35 @@ impl LoopBodyCondPromoter {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ========================================================================
|
||||||
|
// Private Helpers
|
||||||
|
// ========================================================================
|
||||||
|
|
||||||
|
/// Check if statements contain a continue statement
|
||||||
|
fn contains_continue(stmts: &[ASTNode]) -> bool {
|
||||||
|
for stmt in stmts {
|
||||||
|
match stmt {
|
||||||
|
ASTNode::Continue { .. } => return true,
|
||||||
|
ASTNode::If {
|
||||||
|
then_body,
|
||||||
|
else_body,
|
||||||
|
..
|
||||||
|
} => {
|
||||||
|
if Self::contains_continue(then_body) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
if let Some(else_stmts) = else_body {
|
||||||
|
if Self::contains_continue(else_stmts) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
_ => {}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
false
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
|
|||||||
Reference in New Issue
Block a user