Prototype: Template-based code emission

[clank-hayen]

Summary

This PR introduces a template-based code emission system for the Bishop compiler codegen phase.

What's Added

• Template files (`codegen/templates/*.cpp.tmpl`): External template files with `{{PLACEHOLDER}}` syntax
• Template renderer (`template_render.cpp/hpp`): Core rendering functionality with:
  • `render()` - Placeholder substitution
  • `load_template()` - File loading with caching
  • `indent()` / `join_body()` - Indentation helpers

Templates Included

Template	Purpose
`for_range.cpp.tmpl`	Range-based for loop
`for_each.cpp.tmpl`	Foreach loop
`function_def.cpp.tmpl`	Function definition
`struct_def.cpp.tmpl`	Struct definition
`if_stmt.cpp.tmpl`	If statement
`if_else_stmt.cpp.tmpl`	If-else statement

Convenience Functions

```cpp
templates::for_range(var, start, end, body);
templates::for_each(var, collection, body);
templates::function_def(name, params, return_type, body);
templates::struct_def(name, fields);
templates::if_stmt(condition, then_body, else_body);
```

Example Usage

```cpp
std::string code = templates::for_range("i", "0", "n", {
"sum += i;"
});
// Output:
// for (int i = 0; i < n; i++) {
// sum += i;
// }
```

Test plan

• Review template files for correctness
• Verify placeholder substitution works correctly
• Check indentation handling

Related

• Tracking issue: Explore Code Emission Patterns: Templates vs Builders #150
• Compare with builder pattern: feature/codegen-builders

---

🤖 Generated with Claude Code

[clank-hayen]

Code Review: Template-based Code Emission (PR #151)

REVIEW:

Summary: Well-structured prototype with clear separation of concerns, but has critical issues around error handling and path resolution that need addressing before production use.

---

Critical Issues

1. Silent Failure on Template Load (CRITICAL)

Location: template_render.cpp:68-69

if (!file.is_open()) {
    return "";  // Template not found
}

Per project guidelines: "returning empty values instead of proper errors is usually not ok"

When a template file is missing or unreadable, the code silently falls back to inline templates. In production, this could cause:

• Hard-to-debug issues when templates aren't found
• Different behavior between dev and deployment environments

Fix: Return an std::optional<std::string> or throw an exception, and handle the error explicitly at call sites.

2. Hardcoded Relative Paths (CRITICAL)

Location: template_render.cpp:55-61

std::string path = "codegen/templates/" + name + ".cpp.tmpl";
std::ifstream file(path);

if (!file.is_open()) {
    // Try alternative path (when running from build directory)
    path = "../codegen/templates/" + name + ".cpp.tmpl";
    file.open(path);
}

This path resolution is fragile and will break in:

• Installed builds
• Different working directories
• CI environments

Fix: Use a configurable path (environment variable, config file, or compile-time embed the templates as string literals as noted in the comment).

---

Suggestions (Non-blocking)

1. Template Validation

Location: template_render.cpp:22-35

The render() function doesn't validate that all placeholders were substituted. Consider warning on unsubstituted {{...}} patterns remaining in output.

2. Cache Thread Safety

Location: template_render.cpp:17

static std::unordered_map<std::string, std::string> template_cache;

The static cache is not thread-safe. If codegen runs in parallel (e.g., multiple translation units), this could cause data races.

3. Missing Docstrings on Template Files

Per project guidelines, all code requires docstrings. The .cpp.tmpl files have no comments explaining their purpose or placeholder expectations.

---

Comparison with Current Approach

Aspect	Current (fmt::format)	Template-based
Readability	Inline format strings	External files - easier to visualize output
Maintainability	Change requires recompile	Templates can be modified without recompile (dev mode)
Performance	Single string format	File I/O + caching + regex-like substitution
Distribution	Self-contained binary	External file dependency or need to embed
Type Safety	fmt validates at compile time	Runtime string matching

Pros of This Approach

1. ✅ Templates are more readable - you can see the actual C++ output structure
2. ✅ Non-programmers could potentially modify output format
3. ✅ Good docstrings on all C++ functions
4. ✅ Clear namespace organization (codegen::templates)
5. ✅ Convenience functions provide nice API (for_range(), if_stmt(), etc.)

Cons of This Approach

1. ❌ External file dependency introduces deployment complexity
2. ❌ No compile-time validation of placeholder names
3. ❌ Path resolution is fragile
4. ❌ Silent fallback hides configuration errors
5. ❌ Additional I/O overhead (mitigated by caching)

---

Decision: REQUEST_CHANGES

The prototype demonstrates the approach well, but the critical issues around silent failure and path resolution would cause production problems. These should be addressed before this pattern is adopted.

Recommendation for comparing with PR #152 (Builder pattern):

• This approach is better for output format readability
• The builder pattern may be better for type safety and self-contained binaries
• Consider a hybrid: embed templates as string literals at build time (addresses both concerns)

---

🤖 Review generated by Claude Code

[clank-hayen]

Code Review: Template-based Code Emission

REVIEW:
  summary: Clean implementation of template-based codegen with good fallbacks, but has critical error handling issues.

  critical:
    - issue: load_template() returns empty string on failure - violates project rule against returning empty values instead of proper errors
      location: codegen/template_render.cpp:68
      fix: Return std::optional<std::string> or throw an exception. The silent fallback to inline templates masks template loading failures, making debugging difficult.

    - issue: Unvalidated placeholders - missing placeholders silently produce invalid output
      location: codegen/template_render.cpp:22-35
      fix: Track which placeholders exist in template and which were provided. Warn or error on mismatches. A typo like {{VARL}} instead of {{VAR}} would produce broken C++ output with no indication of where it went wrong.

  suggestions:
    - issue: Static template cache has no thread safety
      location: codegen/template_render.cpp:17
      suggestion: If codegen ever runs in parallel, this will race. Consider std::shared_mutex or making cache thread-local.

    - issue: Relative path template loading is fragile
      location: codegen/template_render.cpp:57-65
      suggestion: For production, templates should be embedded at compile time (xxd/cmake embed) or use a configurable base path. Current approach breaks depending on working directory.

    - issue: #include <regex> is unused
      location: codegen/template_render.cpp:13
      suggestion: Remove unused include.

  decision: REQUEST_CHANGES

---

Pros of Template Approach

1. Separation of concerns - C++ output structure lives in .tmpl files, logic in C++
2. Easy to modify output format - Non-developers can adjust templates without touching C++
3. Self-documenting - Template files show exactly what output looks like
4. Good docstrings - Comprehensive documentation throughout

Cons of Template Approach

1. Runtime file I/O - Requires template files to exist at runtime (fragile for deployment)
2. Hidden coupling - Placeholder names couple templates to C++ code without compile-time checks
3. Harder to debug - When output is wrong, you need to check both template AND substitution logic
4. Limited expressiveness - Can't easily do conditional logic within templates (e.g., optional const, different types)

Comparison with Current Approach (fmt::format)

Aspect	Current (fmt::format)	Template-based
Type safety	Compile-time format checks	None - runtime string ops
Debugging	One file to check	Two files to check
Modifiability	Requires C++ knowledge	Templates are plain text
Deployment	Self-contained binary	Requires template files
Complex logic	Easy (it's C++)	Awkward (need multiple templates)

Comparison with Builder Pattern (PR #152)

Aspect	Template-based	Builder pattern
Readability	Templates clear, usage less so	Fluent API very readable
Indentation	Manual (join_body)	Automatic
Nesting validation	None	Runtime checks
Flexibility	Good for static patterns	Better for dynamic content
Error handling	Needs improvement	Needs review

Recommendation

For Bishop's use case (a C++ transpiler with many conditional constructs), I'd lean toward the builder pattern because:

1. Bishop has lots of conditional output (Result wrapping, error handling, type variations)
2. Templates work best for static output patterns, not highly dynamic code
3. The builder's automatic indentation management reduces bugs
4. No runtime file dependencies

However, templates could be valuable for user-facing features like:

• Error message formatting
• Generated boilerplate (main wrapper, test harness)
• Documentation generation

Before Merging

Fix the critical issues:

1. Proper error handling for template loading failures
2. Validation of placeholder substitution
3. Consider compile-time template embedding if this approach is chosen

---

🤖 Generated with Claude Code

[clank-hayen]

Review: Template-based Code Emission Prototype

REVIEW:

Summary: Well-structured template system prototype with good documentation, but has critical issues around error handling and template path resolution that must be addressed before production use.

---

Critical Issues

1. Silent failure on template load (violates project guidelines)

Location: codegen/template_render.cpp:66-67

if (!file.is_open()) {
    return "";  // Template not found
}

Per CLAUDE.md: "returning empty values instead of proper errors is usually not ok"

The current implementation silently returns an empty string when templates fail to load. While the convenience functions fall back to inline templates, this masks real problems (missing templates, permission issues, wrong paths). This should throw an exception or return a std::optional / result type.

Suggested fix: Throw a descriptive error like throw std::runtime_error("Failed to load template: " + name) or use a result type.

2. Template path resolution is fragile

Location: codegen/template_render.cpp:57-64

std::string path = "codegen/templates/" + name + ".cpp.tmpl";
std::ifstream file(path);

if (!file.is_open()) {
    // Try alternative path (when running from build directory)
    path = "../codegen/templates/" + name + ".cpp.tmpl";
    file.open(path);
}

This hardcoded relative path approach is problematic:

• Breaks if the binary is run from a different directory
• Breaks after installation (templates won't be in codegen/templates/ relative to installed binary)
• The comment acknowledges this: "In a production build, templates would be embedded as string literals"

For production, templates should be:

• Embedded at compile time (e.g., using #include with raw string literals or CMake's configure_file)
• Or located via a configurable path/environment variable

---

Suggestions (Non-blocking)

1. Unused placeholder validation

Consider validating that all {{PLACEHOLDER}} markers are substituted. Currently, leftover placeholders silently appear in output. Could add a post-render check.

2. Template caching uses static global

Location: codegen/template_render.cpp:18

static std::unordered_map<std::string, std::string> template_cache;

This is thread-unsafe. If Bishop ever supports parallel compilation, this would be a data race. Consider using thread_local or protecting with a mutex.

3. The #include <regex> is unused

Location: codegen/template_render.cpp:13

The regex header is included but not used. The simple {{KEY}} substitution uses string operations instead.

---

Pros of Template Approach

1. Separation of concerns - Template files are editable without recompilation
2. Readability - Template files show the exact output structure clearly
3. Fallback handling - Inline fallbacks ensure graceful degradation
4. Good documentation - Docstrings are thorough and follow project guidelines
5. Caching - Avoids repeated file I/O

Cons of Template Approach

1. Runtime file dependency - Requires templates to exist at runtime (unless embedded)
2. Debugging complexity - Errors in templates are harder to trace than inline code
3. Path fragility - Current path resolution won't work in all execution contexts
4. Overhead - File I/O and string replacement vs direct fmt::format calls
5. Limited power - Simple substitution can't handle conditionals or loops within templates (the existing if_stmt approach handles this by using separate templates for if/if-else)

Comparison to Current Approach

The existing emit_*.cpp files use fmt::format directly:

string out = fmt::format("for (int {} = {}; {} < {}; {}++) {{\n", var, start, var, end, var);

Current approach advantages:

• No runtime file dependencies
• Faster (no file I/O, caching overhead)
• Single source of truth (code is the template)
• Easier debugging (set breakpoint, see exact string building)

Template approach advantages:

• Templates are more readable for complex structures
• Non-engineers could potentially edit templates
• Changes to output format don't require recompilation (if templates are external)

---

Decision: COMMENT

This is a reasonable prototype for exploration purposes. The core template rendering logic is correct. However, the error handling and path resolution issues would need to be fixed before production use.

For a production decision between this and the Builder pattern (PR #152), I'd recommend:

• If templates will be embedded at compile time: Template approach could work well
• If templates need to be external files: Current implementation needs significant hardening
• If neither requirement exists: The current fmt::format approach is simpler and has fewer moving parts

Looking forward to seeing PR #152 for comparison.

[clank-hayen]

Code Review: Template-based Code Emission Prototype

REVIEW

Summary: Clean, well-documented prototype that demonstrates the template-based approach. The implementation is solid for a prototype but has one notable issue that should be addressed before adoption.

---

Critical Issues

Issue	Location	Fix
Silent failure on missing templates	template_render.cpp:67	The load_template() function returns empty string when template file is not found, violating the project guideline "returning empty values instead of proper errors is usually not ok". While the convenience functions have fallback inline templates, a missing template could silently produce unexpected output if the fallback differs from the external file. Consider throwing an exception or returning a Result<string> type for proper error handling.

---

Suggestions (Non-blocking)

1. Thread safety concern - template_render.cpp:18 - The static template_cache is not thread-safe. If codegen ever runs in parallel (e.g., parallel compilation of multiple files), this could cause data races. Consider using std::mutex or making the cache thread-local.

2. Template path discovery - template_render.cpp:57-64 - The relative path logic (codegen/templates/ and ../codegen/templates/) is fragile. For production, consider:

   • Embedding templates as string literals at build time (as noted in the comment)
   • Using an environment variable or config for template directory
   • Passing template directory as a parameter

3. Unsubstituted placeholder detection - render() doesn't detect if any {{...}} placeholders remain after substitution. This could lead to emitting invalid C++ code silently. Consider adding a check that warns/errors if unreplaced placeholders exist.

4. Missing tests - No unit tests for the template rendering functions. For a prototype this is acceptable, but if this approach is adopted, tests should verify:

   • Placeholder substitution
   • Indentation handling
   • Missing template behavior
   • Edge cases (empty body, nested placeholders)

---

Pros of Template Approach (for comparison with #152)

✅ Readability: Templates look like actual C++ code - easy to see what will be generated
✅ Separation of concerns: Structure (templates) separated from logic (C++ code)
✅ Easy modification: Non-developers can potentially modify output format
✅ Fallback mechanism: Inline fallback templates provide resilience
✅ Caching: Template caching prevents repeated file I/O
✅ Good docstrings: All functions well-documented per project guidelines

Cons of Template Approach

❌ File I/O dependency: External files must be discoverable at runtime
❌ Error handling: String-based, errors detected at runtime not compile time
❌ Limited expressiveness: Complex conditional logic must be handled in C++ code, not templates
❌ Deployment complexity: Template files must be bundled/embedded for distribution

---

Comparison with Current Approach (emit_for.cpp, emit_if.cpp, etc.)

The current fmt::format() approach in files like emit_for.cpp is more direct:

// Current (emit_for.cpp)
string out = fmt::format("for (int {} = {}; {} < {}; {}++) {{\n", var, start, var, end, var);

// Template approach
templates::for_range(var, start, end, body);

The template approach is more readable for complex structures and handles indentation automatically, but adds indirection. For simple patterns, the current approach may be sufficient.

---

Decision

COMMENT - This is a prototype for evaluation against the builder pattern (#152). The implementation is clean and demonstrates the approach well. The silent failure issue should be addressed if this pattern is adopted, but is acceptable for a prototype comparison.

Looking forward to seeing the builder pattern prototype for comparison!

[chrishayen]

I'm definitely not in to the loading a file thing

[clank-hayen]

Thanks for the feedback @chrishayen!

That's a valid concern about the template file loading approach. The file I/O overhead and added complexity of managing external template files may not be worth the benefits.

Current state of the prototypes:

• PR Prototype: Template-based code emission #151 (Templates): Uses external .cpp.tmpl files with placeholder substitution
• PR Prototype: Builder pattern for code emission #152 (Builders): Was closed - used a fluent CppBuilder API with automatic indentation

Based on your feedback, it sounds like neither of these approaches fully address the core need. The templates require file loading, and the builders (while avoiding that) added their own complexity.

Alternative suggestions:

1. Inline string templates - Use C++ raw string literals with placeholders, keep templates in code:

   constexpr auto FOR_LOOP_TEMPLATE = R"cpp(
   for (auto {{ITER}} : {{COLLECTION}}) {
   {{BODY}}
   }
   )cpp";

   • No file I/O
   • Templates still visible and editable
   • Simple string replacement

2. Keep current approach - The existing fmt::format() concatenation works, just needs more consistent formatting helpers

3. Hybrid - Use inline templates for common patterns, keep imperative code for complex logic

Would you like me to explore the inline string templates approach in a new prototype, or would you prefer a different direction?

[clank-hayen]

Code Review

Summary: Well-designed template system with good documentation, but has issues that should be addressed before merging.

Critical Issues (Must Fix)

1. Returning empty string instead of proper error on template load failure

• Location: codegen/template_render.cpp:64-65
• Issue: When a template file cannot be opened, load_template() returns an empty string instead of signaling an error. Per CLAUDE.md: "returning empty values instead of proper errors is usually not ok"
• The fallback mechanism in convenience functions masks this problem, but if someone uses load_template() directly, they get silent failures.
• Fix: Either throw an exception, return std::optional<std::string>, or use a Result type. At minimum, log a warning when falling back to inline templates.

2. Static global cache without thread safety

• Location: codegen/template_render.cpp:17
• Issue: static std::unordered_map<std::string, std::string> template_cache; is accessed without synchronization. If the compiler uses multiple threads (the codebase has async features and channels), this is a data race.
• Fix: Either make the cache thread-local (thread_local), add a mutex, or document that this module is not thread-safe.

Suggestions (Non-blocking)

1. Path traversal consideration (template_render.cpp:55-56): The template name is concatenated directly into a path. While this is internal API, consider validating that name doesn't contain .. or / for defense-in-depth.

2. Missing newline at end of struct template (struct_def.cpp.tmpl): The function_def.cpp.tmpl fallback has a trailing newline but the template file doesn't.

3. No unit tests for the template system: Consider adding tests for render(), indent(), join_body().

---

Decision: REQUEST_CHANGES

The architecture and API design look good. Once the error handling and thread-safety issues are addressed, this is ready to merge.

[clank-hayen]

Code Review

Summary: Solid prototype for template-based code emission with good fallback design.

What's Good

• Well-documented with docstrings throughout (per CLAUDE.md requirements)
• Sensible fallback inline templates if files can't be loaded - graceful degradation
• Clean separation between core rendering (render, load_template, indent, join_body) and convenience functions
• Template caching for performance
• CI passing

Suggestions (non-blocking)

1. Thread safety (for future integration): The static template_cache at line 18 is not thread-safe. Since tests run in parallel with std::async, consider using thread_local or mutex protection when integrating into production. The fallback templates mean this won't cause incorrect behavior, just potentially redundant file reads.

2. Template contract: The .cpp.tmpl files show {{BODY}} with no leading whitespace, relying on join_body() to add indentation. This coupling is implicit - consider adding a comment documenting this contract.

Security Check

• No injection vulnerabilities - template substitution is straightforward string replacement
• No file path traversal risks - template names are hardcoded
• No sensitive data exposure

Verdict: Code is correct, well-documented, and ready for integration. No blocking issues found. ✅

[clank-hayen]

Closing based on owner feedback - the file loading approach adds unwanted complexity. The current fmt::format() approach will be retained.