Experiment: Add #ifdef preprocessor for conditional XDR codegen (Go)#227
Experiment: Add #ifdef preprocessor for conditional XDR codegen (Go)#227leighmcculloch wants to merge 2 commits intomasterfrom
Conversation
leighmcculloch
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds experimental support for conditional compilation in XDR source files using #ifdef, #else, and #endif preprocessor directives. The implementation introduces a preprocessor that strips directives while tracking conditional blocks, annotates AST nodes with their ifdef conditions, and extends the Go generator to output separate build-tagged files for each feature flag combination. Types without conditionals remain in the main file, while types with conditional members are split across multiple files with appropriate Go build tags.
Changes:
- Adds a preprocessor module that parses ifdef directives and annotates AST nodes with conditional information
- Extends the Go generator to produce multiple build-tagged files based on feature flag combinations
- Relaxes enum grammar to accept optional trailing commas (to handle ifdef-related edge cases)
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/xdrgen/preprocessor.rb | New preprocessor that parses ifdef directives, tracks conditions by line, and annotates AST nodes |
| lib/xdrgen/ast/definitions/base.rb | Adds ifdefs attribute to AST definition nodes |
| lib/xdrgen/compilation.rb | Integrates preprocessor into compilation flow, processes source before parsing |
| lib/xdrgen/grammar/enum.treetop | Makes commas optional between enum members and allows trailing commas |
| lib/xdrgen/generators/go.rb | Extensive changes to support multi-file generation with build tags based on ifdef conditions |
| lib/xdrgen.rb | Autoloads the new Preprocessor class |
| spec/fixtures/generator/ifdef.x | Test fixture demonstrating ifdef usage with enums, structs, unions, and else branches |
| spec/lib/xdrgen/generator_spec.rb | Skips ifdef.x test for non-Go generators |
| spec/output/generator_spec_go/ifdef.x/* | Generated Go files with build tags for feature combinations |
| spec/output/generator_spec_ruby/ifdef.x/* | Ruby output files (for inspection only, tests are skipped) |
| spec/output/generator_spec_javascript/ifdef.x/* | JavaScript output files (for inspection only, tests are skipped) |
| spec/output/generator_spec_elixir/ifdef.x/* | Elixir output files (for inspection only, tests are skipped) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| space? | ||
| first_member_n:enum_member | ||
| additional_members_n:(space? "," space? enum_member )* | ||
| additional_members_n:(space? ","? space? enum_member )* |
There was a problem hiding this comment.
Making the comma optional between enum members (","?) could allow invalid XDR syntax to be accepted. Standard XDR requires commas between enum members. This change appears to be made to handle ifdef directives that might leave trailing or missing commas, but it relaxes the grammar beyond what's necessary. Consider whether this is intentional or if there's a better way to handle the ifdef case without accepting invalid syntax.
| additional_members_n:(space? ","? space? enum_member )* | |
| additional_members_n:(space? "," space? enum_member )* |
| defn.normal_arms.each do |a| | ||
| all_conds = conditions_at_offset(a.interval.first) | ||
| a.ifdefs = all_conds.drop(defn_conditions.length) | ||
| end |
There was a problem hiding this comment.
Union default arms are not annotated with ifdef conditions. If a union has a default arm that is inside an ifdef block, it will not be properly annotated. The code only annotates normal_arms but not default_arm. Consider adding annotation for the default arm if it exists.
| end | |
| end | |
| if defn.respond_to?(:default_arm) && defn.default_arm | |
| all_conds = conditions_at_offset(defn.default_arm.interval.first) | |
| defn.default_arm.ifdefs = all_conds.drop(defn_conditions.length) | |
| end |
| # Returns the effective arms for a union, applying the active filter | ||
| def effective_arms(union) | ||
| arms = union.normal_arms | ||
| arms = arms.select { |a| @active_member_filter.call(a) } if @active_member_filter | ||
| # Always include the default arm if present | ||
| if union.default_arm.present? | ||
| arms + [union.default_arm] | ||
| else | ||
| arms | ||
| end | ||
| end |
There was a problem hiding this comment.
The effective_arms method always includes the default arm without applying the member filter. If a union's default arm is inside an ifdef block, it will be incorrectly included in all build-tagged variants. The default arm should also be checked against the member filter when one is active.
| def process | ||
| lines = @original_source.split("\n", -1) | ||
| cleaned_lines = [] | ||
| ifdef_stack = [] | ||
|
|
||
| lines.each_with_index do |line, i| | ||
| stripped = line.strip | ||
| if stripped =~ /\A%?\s*#ifdef\s+(\w+)/ | ||
| ifdef_stack.push(IfdefCondition.new($1, false)) | ||
| cleaned_lines << (" " * line.length) | ||
| elsif stripped =~ /\A%?\s*#else\b/ | ||
| if ifdef_stack.any? | ||
| top = ifdef_stack.last | ||
| ifdef_stack[-1] = IfdefCondition.new(top.name, !top.negated) | ||
| end | ||
| cleaned_lines << (" " * line.length) | ||
| elsif stripped =~ /\A%?\s*#endif\b/ | ||
| ifdef_stack.pop if ifdef_stack.any? | ||
| cleaned_lines << (" " * line.length) | ||
| else | ||
| @line_conditions[i] = ifdef_stack.map { |c| IfdefCondition.new(c.name, c.negated) } if ifdef_stack.any? | ||
| cleaned_lines << line | ||
| end | ||
| end |
There was a problem hiding this comment.
Missing error handling for unbalanced ifdef/else/endif directives. If the source has an unmatched ifdef or endif, the preprocessor silently accepts it and may produce incorrect results. For example, an extra endif will be ignored (line 44 checks "if ifdef_stack.any?" before popping), and a missing endif will leave items on the stack. Consider adding validation to ensure directives are properly balanced.
| def render_enum(out, enum) | ||
| members = effective_enum_members(enum) | ||
|
|
||
| # render the "enum" | ||
| out.puts "type #{name enum} int32" | ||
| out.puts "const (" | ||
| out.indent do | ||
| first_member = enum.members.first | ||
| first_member = members.first | ||
| out.puts "#{name enum}#{name first_member} #{name enum} = #{first_member.value}" |
There was a problem hiding this comment.
Potential nil pointer error when all enum members are filtered out. The code calls members.first without checking if members is empty. If a feature flag combination results in an enum with all members filtered out, this will cause a nil pointer error. Consider adding a check or ensuring that enums always have at least one member in all variants.
|
|
||
| def render_union(out, union) | ||
| arms = effective_arms(union) | ||
| normal_arms = effective_normal_arms(union) |
There was a problem hiding this comment.
This assignment to normal_arms is useless, since its value is never read.
| normal_arms = effective_normal_arms(union) | |
| effective_normal_arms(union) |
1 participant
What
Add a preprocessor that parses
#ifdef,#else, and#endifdirectives in XDR source files. The preprocessor strips directives before parsing, tracks which lines are conditional, and annotates AST nodes (definitions, struct members, enum members, union arms) with their ifdef conditions. The Go generator uses these annotations to split conditional types into separate files with//go:buildtags, producing one file per feature-flag combination. Types with no ifdefs remain in the main file. The enum grammar is relaxed to allow optional trailing commas.Why
This is an experiment for: