Open
Conversation
Co-authored-by: sonaldcao <sonaldcao@gmail.com>
审查者指南此 PR 通过完全实现先前未实现的 AST 遍历器来支持核心 C99 构造（包括字符字面量、sizeof、三元表达式、类型转换、复合赋值、复合字面量和初始化列表），从而扩展了 LLVM 代码生成器，并添加了元数据和测试文件来验证这些功能。 三元运算符 (?:) 代码生成的序列图sequenceDiagram
participant AstWalker as AstWalker
participant Builder as LLVMBuilder
participant Context as WalkContext
AstWalker->>AstWalker: WalkConditionalOperation(cond ? true : false)
AstWalker->>Builder: Create condition value
Builder-->>AstWalker: cond (i1)
AstWalker->>Builder: CreateCondBr(cond, trueBB, falseBB)
AstWalker->>AstWalker: WalkAst(trueExpr)
AstWalker->>Builder: CreateLoad if pointer
AstWalker->>AstWalker: WalkAst(falseExpr)
AstWalker->>Builder: CreateLoad if pointer
AstWalker->>Builder: CreateBr(mergeBB) for both branches
AstWalker->>Builder: CreatePHI(resultType, [trueVal, falseVal])
Builder-->>Context: PHI node (merged value)
Context-->>AstWalker: Set ctx.Value = PHI
LLVM 代码生成中扩展的 AST 遍历器的类图classDiagram
class AstWalker {
+WalkCharLiteralExpr()
+WalkSizeofExpr()
+WalkConditionalOperation()
+WalkCastExpr()
+WalkCompoundAssignExpr()
+WalkCompoundLiteralExpr()
+WalkInitListExpr()
}
class WalkContext {
+Value: llvm.Value
}
class llvm.Value
class llvm.Type
AstWalker --> WalkContext
WalkContext --> llvm.Value
AstWalker ..> llvm.Type : uses
AstWalker ..> llvm.Value : uses
复合赋值运算符处理的类图classDiagram
class AstWalker {
+WalkCompoundAssignExpr()
}
class lexer {
<<enumeration>>
MINUS_ASSIGN
MUL_ASSIGN
DIV_ASSIGN
MOD_ASSIGN
AND_ASSIGN
OR_ASSIGN
XOR_ASSIGN
LSHIFT_ASSIGN
RSHIFT_ASSIGN
}
AstWalker ..> lexer : uses
复合字面量和初始化列表支持的类图classDiagram
class AstWalker {
+WalkCompoundLiteralExpr()
+WalkInitListExpr()
}
class llvm.Value
class llvm.Type
AstWalker ..> llvm.Value : uses
AstWalker ..> llvm.Type : uses
文件级别更改
提示和命令与 Sourcery 交互
自定义您的体验访问您的 dashboard 以：
获取帮助
Original review guide in EnglishReviewer's GuideThis PR extends the LLVM code generator by fully implementing previously unimplemented AST walkers to support core C99 constructs—covering character literals, sizeof, ternary expressions, type casts, compound assignments, compound literals, and initialization lists—and adds metadata and test files to validate these features. Sequence diagram for ternary operator (?:) code generationsequenceDiagram
participant AstWalker as AstWalker
participant Builder as LLVMBuilder
participant Context as WalkContext
AstWalker->>AstWalker: WalkConditionalOperation(cond ? true : false)
AstWalker->>Builder: Create condition value
Builder-->>AstWalker: cond (i1)
AstWalker->>Builder: CreateCondBr(cond, trueBB, falseBB)
AstWalker->>AstWalker: WalkAst(trueExpr)
AstWalker->>Builder: CreateLoad if pointer
AstWalker->>AstWalker: WalkAst(falseExpr)
AstWalker->>Builder: CreateLoad if pointer
AstWalker->>Builder: CreateBr(mergeBB) for both branches
AstWalker->>Builder: CreatePHI(resultType, [trueVal, falseVal])
Builder-->>Context: PHI node (merged value)
Context-->>AstWalker: Set ctx.Value = PHI
Class diagram for extended AST walkers in LLVM codegenclassDiagram
class AstWalker {
+WalkCharLiteralExpr()
+WalkSizeofExpr()
+WalkConditionalOperation()
+WalkCastExpr()
+WalkCompoundAssignExpr()
+WalkCompoundLiteralExpr()
+WalkInitListExpr()
}
class WalkContext {
+Value: llvm.Value
}
class llvm.Value
class llvm.Type
AstWalker --> WalkContext
WalkContext --> llvm.Value
AstWalker ..> llvm.Type : uses
AstWalker ..> llvm.Value : uses
Class diagram for compound assignment operator handlingclassDiagram
class AstWalker {
+WalkCompoundAssignExpr()
}
class lexer {
<<enumeration>>
MINUS_ASSIGN
MUL_ASSIGN
DIV_ASSIGN
MOD_ASSIGN
AND_ASSIGN
OR_ASSIGN
XOR_ASSIGN
LSHIFT_ASSIGN
RSHIFT_ASSIGN
}
AstWalker ..> lexer : uses
Class diagram for compound literal and initializer list supportclassDiagram
class AstWalker {
+WalkCompoundLiteralExpr()
+WalkInitListExpr()
}
class llvm.Value
class llvm.Type
AstWalker ..> llvm.Value : uses
AstWalker ..> llvm.Type : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @sonald - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/github.com/sonald/sc/codegen/llvm_cg.go:850` </location>
<code_context>
+ walker.Info.builder.CreateStore(val, lhs)
+ op = val
+ case lexer.MINUS_ASSIGN:
+ var l, r llvm.Value = lhs, rhs
+ if l.Type().TypeKind() == llvm.PointerTypeKind {
</code_context>
<issue_to_address>
Consider extracting repeated pointer loading and operator logic in binary and compound assignment walkers into reusable helper functions and operator maps.
Here are a few quick wins to collapse the enormous `WalkBinaryOperation`/`WalkCompoundAssignExpr` plumbing into small, reusable helpers. All existing functionality is preserved.
1. Pull your “load if pointer” pattern into one helper:
```go
// loadIfPtr loads *v if it's a pointer, otherwise returns v unchanged.
func loadIfPtr(v llvm.Value, b llvm.Builder) llvm.Value {
if v.Type().TypeKind() == llvm.PointerTypeKind {
return b.CreateLoad(v, "")
}
return v
}
```
2. Predeclare your operator‐to‐builder maps at package scope (rather than in each walker):
```go
var intArithOps = map[lexer.Kind]func(b llvm.Builder, l, r llvm.Value) llvm.Value{
lexer.PLUS: func(b llvm.Builder, l, r llvm.Value) llvm.Value { return b.CreateAdd(l, r, "") },
lexer.MINUS: func(b llvm.Builder, l, r llvm.Value) llvm.Value { return b.CreateSub(l, r, "") },
lexer.MUL: func(b llvm.Builder, l, r llvm.Value) llvm.Value { return b.CreateMul(l, r, "") },
lexer.DIV: func(b llvm.Builder, l, r llvm.Value) llvm.Value { return b.CreateSDiv(l, r, "") },
lexer.MOD: func(b llvm.Builder, l, r llvm.Value) llvm.Value { return b.CreateSRem(l, r, "") },
}
var intCmpOps = map[lexer.Kind]llvm.IntPredicate{
lexer.GREAT: llvm.IntSGT,
lexer.GE: llvm.IntSGE,
lexer.LESS: llvm.IntSLT,
lexer.LE: llvm.IntSLE,
lexer.NE: llvm.IntNE,
lexer.EQUAL: llvm.IntEQ,
}
```
3. Replace the huge switch in `WalkBinaryOperation` with:
```go
walker.WalkBinaryOperation = func(ws ast.WalkStage, e *ast.BinaryOperation, ctx *ast.WalkContext) bool {
if ws != ast.WalkerPropagate || InSwitchCaseCounting() {
return true
}
b := walker.Info.builder
lhs := loadIfPtr(ast.WalkAst(e.LHS, walker, ctx).(llvm.Value), b)
rhs := loadIfPtr(ast.WalkAst(e.RHS, walker, ctx).(llvm.Value), b)
// arithmetic
if fn, ok := intArithOps[e.Op]; ok {
ctx.Value = fn(b, lhs, rhs)
return false
}
// comparisons
if pred, ok := intCmpOps[e.Op]; ok {
ctx.Value = b.CreateICmp(pred, lhs, rhs, "")
return false
}
// TODO: logical, bitwise, comma, assign, etc. can use the same pattern:
// 1) map lexer.Kind to a small func
// 2) call it here
// 3) return false
panic("unhandled binary op " + e.Op.String())
}
```
4. Do the same in `WalkCompoundAssignExpr`:
```go
walker.WalkCompoundAssignExpr = func(ws ast.WalkStage, e *ast.CompoundAssignExpr, ctx *ast.WalkContext) bool {
if ws != ast.WalkerPropagate {
return true
}
b := walker.Info.builder
lhsPtr := ast.WalkAst(e.LHS, walker, ctx).(llvm.Value)
rhsPtr := ast.WalkAst(e.RHS, walker, ctx).(llvm.Value)
l := loadIfPtr(lhsPtr, b)
r := loadIfPtr(rhsPtr, b)
// e.g. for +=
switch e.Op {
case lexer.PLUS_ASSIGN:
val := intArithOps[lexer.PLUS](b, l, r)
b.CreateStore(val, lhsPtr)
ctx.Value = val
default:
panic("not implemented: " + e.Op.String())
}
return false
}
```
Once you’ve extracted these patterns you can remove all the repeated `if TypeKind() == Pointer` blocks and the huge inline maps, and focus each walker on just orchestrating the right helpers.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| case lexer.MINUS_ASSIGN: |
There was a problem hiding this comment.
issue (complexity): Consider extracting repeated pointer loading and operator logic in binary and compound assignment walkers into reusable helper functions and operator maps.
Here are a few quick wins to collapse the enormous WalkBinaryOperation/WalkCompoundAssignExpr plumbing into small, reusable helpers. All existing functionality is preserved.
- Pull your “load if pointer” pattern into one helper:
// loadIfPtr loads *v if it's a pointer, otherwise returns v unchanged.
func loadIfPtr(v llvm.Value, b llvm.Builder) llvm.Value {
if v.Type().TypeKind() == llvm.PointerTypeKind {
return b.CreateLoad(v, "")
}
return v
}- Predeclare your operator‐to‐builder maps at package scope (rather than in each walker):
var intArithOps = map[lexer.Kind]func(b llvm.Builder, l, r llvm.Value) llvm.Value{
lexer.PLUS: func(b llvm.Builder, l, r llvm.Value) llvm.Value { return b.CreateAdd(l, r, "") },
lexer.MINUS: func(b llvm.Builder, l, r llvm.Value) llvm.Value { return b.CreateSub(l, r, "") },
lexer.MUL: func(b llvm.Builder, l, r llvm.Value) llvm.Value { return b.CreateMul(l, r, "") },
lexer.DIV: func(b llvm.Builder, l, r llvm.Value) llvm.Value { return b.CreateSDiv(l, r, "") },
lexer.MOD: func(b llvm.Builder, l, r llvm.Value) llvm.Value { return b.CreateSRem(l, r, "") },
}
var intCmpOps = map[lexer.Kind]llvm.IntPredicate{
lexer.GREAT: llvm.IntSGT,
lexer.GE: llvm.IntSGE,
lexer.LESS: llvm.IntSLT,
lexer.LE: llvm.IntSLE,
lexer.NE: llvm.IntNE,
lexer.EQUAL: llvm.IntEQ,
}- Replace the huge switch in
WalkBinaryOperationwith:
walker.WalkBinaryOperation = func(ws ast.WalkStage, e *ast.BinaryOperation, ctx *ast.WalkContext) bool {
if ws != ast.WalkerPropagate || InSwitchCaseCounting() {
return true
}
b := walker.Info.builder
lhs := loadIfPtr(ast.WalkAst(e.LHS, walker, ctx).(llvm.Value), b)
rhs := loadIfPtr(ast.WalkAst(e.RHS, walker, ctx).(llvm.Value), b)
// arithmetic
if fn, ok := intArithOps[e.Op]; ok {
ctx.Value = fn(b, lhs, rhs)
return false
}
// comparisons
if pred, ok := intCmpOps[e.Op]; ok {
ctx.Value = b.CreateICmp(pred, lhs, rhs, "")
return false
}
// TODO: logical, bitwise, comma, assign, etc. can use the same pattern:
// 1) map lexer.Kind to a small func
// 2) call it here
// 3) return false
panic("unhandled binary op " + e.Op.String())
}- Do the same in
WalkCompoundAssignExpr:
walker.WalkCompoundAssignExpr = func(ws ast.WalkStage, e *ast.CompoundAssignExpr, ctx *ast.WalkContext) bool {
if ws != ast.WalkerPropagate {
return true
}
b := walker.Info.builder
lhsPtr := ast.WalkAst(e.LHS, walker, ctx).(llvm.Value)
rhsPtr := ast.WalkAst(e.RHS, walker, ctx).(llvm.Value)
l := loadIfPtr(lhsPtr, b)
r := loadIfPtr(rhsPtr, b)
// e.g. for +=
switch e.Op {
case lexer.PLUS_ASSIGN:
val := intArithOps[lexer.PLUS](b, l, r)
b.CreateStore(val, lhsPtr)
ctx.Value = val
default:
panic("not implemented: " + e.Op.String())
}
return false
}Once you’ve extracted these patterns you can remove all the repeated if TypeKind() == Pointer blocks and the huge inline maps, and focus each walker on just orchestrating the right helpers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement core C99 code generation features to resolve 'not implemented' panics and improve compiler completeness.
This PR addresses the initial phase of implementing missing functionalities identified in the compiler project. It focuses on the code generation module, specifically:
sizeof, ternary operator?:, type casting, character literals).-=,*=,/=,%=,&=,|=,^=,<<=,>>=).These implementations eliminate several 'not implemented' panics and significantly advance the compiler's ability to generate LLVM IR for common C99 constructs.
好的，这是将 pull request 总结翻译成中文的结果：
Sourcery 总结
在 LLVM 后端实现缺失的 C99 代码生成功能，以消除“未实现”的 panic 并提高编译器的完整性
新功能：
构建：
文档：
测试：
Original summary in English
Summary by Sourcery
Implement missing C99 code generation features in LLVM backend to eliminate "not implemented" panics and improve compiler completeness
New Features:
Build:
Documentation:
Tests: