Prototype: Builder pattern for code emission#152
Conversation
Introduces CppBuilder class with fluent API for generating C++ code: - Automatic indentation management - Methods for common patterns (for_range, for_each, if_stmt, etc.) - Block open/close with nesting depth tracking - Catches mismatched braces at runtime with exceptions - Convenience functions for one-shot generation Key methods: - for_range(var, start, end) / for_each(var, collection) - if_stmt(condition) / else_if(condition) / else_block() - function(return_type, name, params) - struct_def(name) / field(type, name) / end_struct() - statement(stmt) / return_stmt(expr) - block() / end() This is a prototype for exploring builder-based code generation as an alternative to inline string concatenation. Refs: #150 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![clank-hayen[bot]](https://avatars.githubusercontent.com/in/2555175?s=60&v=4){kind=small}
There was a problem hiding this comment.
REVIEW: Builder Pattern Prototype
Summary: Well-structured fluent builder with good fundamentals, but the convenience functions have issues and the prototype isn't integrated.
Critical Issues
1. Convenience functions don't use the builder properly
Location: codegen/cpp_builder.cpp:333-356 (and other convenience functions)
The convenience functions (for_range_stmt, for_each_stmt, etc.) bypass the builder's indentation system entirely and hardcode "\t" for indentation:
std::string for_range_stmt(...) {
CppBuilder b;
b.for_range(var, start, end);
for (const auto& stmt : body) {
b.raw(b.str().empty() ? "" : ""); // no-op, just need to add statements
b.line(stmt);
}
// Fix: we need to manually add statements
CppBuilder builder;
builder.raw("for (int " + var + " = " + start + ...); // bypasses builder
for (const auto& stmt : body) {
builder.raw("\t" + stmt + "\n"); // hardcoded indent
}
builder.raw("}");
return builder.str();
}This is confusing code - it creates a builder, abandons it, creates another, and then uses raw() to bypass all the fluent API advantages. The convenience functions should use the builder properly:
std::string for_range_stmt(...) {
CppBuilder b;
b.for_range(var, start, end);
for (const auto& stmt : body) {
b.statement(stmt);
}
b.end();
return b.str();
}2. Nesting depth can go negative silently
Location: codegen/cpp_builder.cpp:318-323
CppBuilder& CppBuilder::dedent() {
if (indent_level_ > 0) {
--indent_level_;
}
return *this;
}This silently clamps to 0 rather than throwing an error like end() does. Inconsistent error handling - if end() throws on mismatched blocks, dedent() should throw on underflow or at least match the behavior.
Suggestions (Non-blocking)
1. No integration with existing codegen
The builder isn't used anywhere in the codebase. The current emit functions in emit_for.cpp, emit_if.cpp, etc. use fmt::format with manual string concatenation. For this prototype to be properly evaluated, it would help to see at least one emit function converted to use the builder.
2. Missing const auto& option for for_each
for_each() hardcodes auto&, but read-only iteration should use const auto&. Consider:
CppBuilder& for_each(const std::string& var, const std::string& collection, bool is_const = false);3. depth() exposed but never used
The depth() method is public for "debugging" but there's no assertion or validation that depth returns to 0 after generating complete code. Consider adding a validate() or is_complete() method.
4. No tests included
A prototype should include at least basic unit tests demonstrating the API works correctly. This would also serve as documentation of expected usage.
Comparison with Current Approach
| Aspect | Current (fmt::format) |
Builder Pattern |
|---|---|---|
| Readability | Explicit string building | Fluent method chains |
| Indentation | Manual ("\t") |
Automatic tracking |
| Brace matching | Manual (error-prone) | Runtime validation |
| Nested structures | Harder to visualize | Natural nesting |
| Performance | Direct fmt::format |
ostringstream buffering |
| Integration | Already in use | Would require migration |
Pros of Builder:
- Automatic indentation is a real improvement - no more counting tabs
- Runtime validation catches mismatched braces (current approach doesn't)
- Fluent API reads more like the output structure
Cons of Builder:
- Requires learning new API
end()calls can be easy to forget (though the runtime check catches this)- The convenience functions demonstrate the API friction - they bypass the builder
Decision: COMMENT
This is a reasonable prototype that demonstrates the builder pattern approach. The core CppBuilder class is solid, but the convenience functions have bugs that need fixing before the pattern can be fully evaluated.
For comparing with PR #151 (templates), I'd recommend:
- Fix the convenience functions to actually use the builder
- Convert one real emit function (e.g.,
emit_for.cpp) to use the builder - Add basic tests
This will give a fair comparison of how each pattern handles real codegen scenarios.
There was a problem hiding this comment.
REVIEW: Builder Pattern for Code Emission
Summary: Well-implemented fluent builder API with solid error handling, but the convenience functions reveal some design limitations.
✅ Strengths
1. Fluent API Design
The method chaining is clean and intuitive:
b.function("int", "sum", "int n")
.for_range("i", "0", "n")
.statement("sum += i")
.end()
.end();2. Automatic Indentation Management
Eliminates manual \t concatenation seen in current codegen. Indentation is correctly tracked via indent_level_.
3. Runtime Safety for Block Nesting
Good use of nesting_depth_ to throw on mismatched blocks:
if (nesting_depth_ <= 0) {
throw std::runtime_error("CppBuilder::end() called with no open block");
}4. Good Documentation
All methods have docstrings as required by project guidelines. Header has clear usage example.
⚠️ Issues to Address
1. Convenience Functions Don't Use the Builder
The convenience functions (for_range_stmt, for_each_stmt, etc.) bypass the fluent API and use raw string concatenation:
// Lines 347-354 in cpp_builder.cpp
CppBuilder builder;
builder.raw("for (int " + var + " = " + start + ...);
for (const auto& stmt : body) {
builder.raw("\t" + stmt + "\n"); // Hardcoded \t
}This defeats the purpose of the builder pattern. If we're using raw concatenation anyway, why have the builder?
Fix: Either:
- Use the fluent API in convenience functions:
builder.for_range(...).statement(...).end() - Remove convenience functions (prefer fluent API only)
- Accept that convenience functions are thin wrappers for simple cases
2. Dead Code in for_range_stmt
Lines 337-345 create a builder b, do nothing useful, then create a new builder:
CppBuilder b;
b.for_range(var, start, end);
for (const auto& stmt : body) {
b.raw(b.str().empty() ? "" : ""); // no-op
b.line(stmt);
}
// Fix: we need to manually add statements
CppBuilder builder; // starts over!This looks like unfinished refactoring. Should be cleaned up.
3. Nested Indentation Not Handled in Convenience Functions
The convenience functions hardcode single-level indentation (\t). For nested structures, this won't work:
// If you nest a for_range inside a function_def, body gets \t not \t\t💭 Comparison with Template Pattern (PR #151)
| Aspect | Builder (this PR) | Template (#151) |
|---|---|---|
| Code Shape Visibility | Methods obscure final shape | Templates show exact output |
| Extensibility | Add new methods | Add new template files |
| Nested Structures | Natural via chaining | Requires careful indentation |
| Error Detection | Runtime (block mismatch) | Runtime (missing placeholder) |
| File I/O | None | Template loading |
| IDE Support | Autocomplete on methods | Syntax highlighting in templates |
Builder Pros:
- Single class, no external files
- Method chaining for complex nesting
- Type-safe API (method names vs string placeholders)
Builder Cons:
- Harder to visualize final C++ output
- More verbose for simple cases
- The convenience functions show the pattern gets awkward when you want one-shot generation
📋 Suggestions (Non-blocking)
- Consider removing convenience functions or making them properly use the fluent API
- Add a
validate()method to checknesting_depth_ == 0before callingstr() - The
for_rangemethod hardcodesintfor loop variable - consider parameterizing this
Decision: COMMENT
No critical bugs or security issues. This is a valid prototype for evaluation against the template approach. The convenience function issues should be addressed if this pattern is chosen for adoption, but they're fine for a prototype.
The choice between this and templates should depend on which pattern the team finds easier to read and extend. I'd lean slightly toward templates for visibility of final output, but the builder pattern's nested block handling is genuinely useful for complex structures.
🤖 Generated with Claude Code
REVIEW: Builder Pattern PrototypeSummary: Solid fluent builder API design with good core implementation, but convenience functions defeat the pattern's purpose. Critical Issues1. Convenience functions bypass the builder pattern entirely Location: All convenience functions (
Example from CppBuilder builder;
builder.raw("for (int " + var + " = " + start + "; " + var + " < " + end + "; " + var + "++) {\n");
for (const auto& stmt : body) {
builder.raw("\t" + stmt + "\n"); // hardcoded indent, ignores builder's indentation
}
builder.raw("}");The correct implementation should use the fluent API: std::string for_range_stmt(const std::string& var, const std::string& start,
const std::string& end, const std::vector<std::string>& body) {
CppBuilder b;
b.for_range(var, start, end);
for (const auto& stmt : body) {
b.statement(stmt);
}
b.end();
return b.str();
}2. Dead code in Location: CppBuilder b;
b.for_range(var, start, end);
for (const auto& stmt : body) {
b.raw(b.str().empty() ? "" : ""); // no-op
b.line(stmt);
}
// Fix: we need to manually add statements
CppBuilder builder; // Discards b and starts freshThis creates a builder Suggestions (Non-blocking)1.
CppBuilder& CppBuilder::dedent() {
if (indent_level_ <= 0) {
throw std::runtime_error("CppBuilder::dedent() called at indent level 0");
}
--indent_level_;
return *this;
}2. No validation that code is complete There's no way to verify std::string str_validated() const {
if (nesting_depth_ != 0) {
throw std::runtime_error("CppBuilder has " + std::to_string(nesting_depth_) + " unclosed blocks");
}
return str();
}3. Read-only iteration should use 4. The loop type is fixed to CppBuilder& for_range(const std::string& var, const std::string& start,
const std::string& end, const std::string& type = "int");5. Not integrated with existing codegen The builder isn't used anywhere in the codebase. For fair comparison with PR #151, it would help to convert at least one emit function (e.g., Comparison Assessment
The builder pattern's core value proposition (automatic indentation + block validation) is sound. The fluent API is well-designed and the Decision: This is a valid prototype for comparison with the template approach in PR #151. The core
For comparing prototypes, I'd recommend fixing the convenience functions first so both PRs demonstrate their patterns cleanly. 🤖 Generated with Claude Code |
REVIEW: Builder Pattern for Code EmissionSummary: Well-designed fluent builder API with automatic indentation management, but convenience functions have implementation issues and the pattern may add complexity without solving the core problem. ✅ Strengths1. Clean Fluent API b.function("int", "sum_range", "int n")
.statement("int sum = 0")
.for_range("i", "0", "n")
.statement("sum += i")
.end()
.return_stmt("sum")
.end();2. Runtime Block Validation 3. Automatic Indentation 4. Good Documentation ❌ IssuesCritical: Convenience functions bypass the builder The convenience functions (lines 333-436) have problems:
🔶 Architectural Concerns1. Doesn't address nested AST emission The current codegen works by recursively calling For example, for (const auto& stmt : fn.body) {
body.push_back(generate_statement(state, *stmt)); // Returns string
}Using CppBuilder would require either:
2. Comparison with current approach Current (emit_for.cpp): string out = fmt::format("for (int {} = {}; {} < {}; {}++) {{\n", ...);
for (const auto& stmt : body) { out += "\t" + stmt + "\n"; }
out += "}";Builder convenience function (cpp_builder.cpp): builder.raw("for (int " + var + " = " + start + ...);
for (const auto& stmt : body) { builder.raw("\t" + stmt + "\n"); }
builder.raw("}");These are functionally identical. The fluent API is nicer but the convenience functions that would be used in practice don't leverage it. 📋 Suggestions
Decision: COMMENTThis is a reasonable prototype for exploration. The fluent API design is good, but the implementation has issues and it's unclear how the builder would integrate with the existing recursive codegen architecture without significant refactoring. Recommend comparing this against the template pattern (PR #151) before deciding which approach to pursue. The key question is: how does each pattern handle nested AST emission? |
2 participants
Summary
This PR introduces a fluent builder pattern for C++ code emission in the Bishop compiler codegen phase.
What's Added
Key Methods
Example Usage
```cpp
CppBuilder b;
b.function("int", "sum_range", "int n")
.statement("int sum = 0")
.for_range("i", "0", "n")
.statement("sum += i")
.end()
.return_stmt("sum")
.end();
std::string code = b.str();
// Output:
// int sum_range(int n) {
// int sum = 0;
// for (int i = 0; i < n; i++) {
// sum += i;
// }
// return sum;
// }
```
Convenience Functions
For one-shot generation without the builder:
```cpp
builder::for_range_stmt(var, start, end, body);
builder::for_each_stmt(var, collection, body);
builder::function_def(return_type, name, params, body);
builder::struct_def(name, fields);
builder::if_stmt(condition, then_body, else_body);
```
Test plan
Related
🤖 Generated with Claude Code