test: migrate _testdata IR cases to source checks#1780
Conversation
There was a problem hiding this comment.
Code Review
This pull request migrates LLVM IR test expectations from standalone files directly into the Go source files using LITTEST and CHECK directives. Feedback identifies a bug in cl/_testdata/debug/in.go where the CHECK directive for globalInt expects a value of 0 despite the variable being initialized to 301, which will cause the test to fail.
| // CHECK: @"{{.*}}.globalInt" = global i64 0, align 8, !dbg ! | ||
| var globalInt int = 301 |
There was a problem hiding this comment.
Review Summary
Clean and well-structured migration from out.ll baseline files to source-embedded LITTEST/CHECK assertions. The approach is consistent across most files and the CHECK directives correctly verify key IR lowering points. A few consistency issues and one missed deletion noted below.
Key Findings:
- Orphaned
out.ll:embedunexport/out.llwas not deleted despite the source file having the// LITTESTmarker - Inconsistent wildcard usage:
embedunexport.gouses fully qualified package names while all other 21 files use{{.*}}wildcards - Mixed style in
foo.go: Mixes fully qualifiedruntime.efacetype names with{{.*}}wildcards in the same file - Detached CHECK blocks in
debug/in.go: CHECK blocks forFuncWithAllTypeParamsandFuncWithAllTypeStructParamare placed before the unrelatedScopeIffunction, which could confuse readers
No security or performance concerns.
| @@ -7,23 +8,57 @@ type Object interface { | |||
| } | |||
|
|
|||
| // Base implements Object | |||
| // CHECK: %"github.com/goplus/llgo/cl/_testdata/embedunexport.Base" = type { %"github.com/goplus/llgo/runtime/internal/runtime.String" } | |||
There was a problem hiding this comment.
This file uses fully qualified package names (e.g., "github.com/goplus/llgo/cl/_testdata/embedunexport.Base") while all other 21 migrated files use {{.*}} wildcards. This makes the file fragile to package path changes and inconsistent with the rest of the migration.
Also, the corresponding out.ll file was not deleted for this directory — it's now dead code since LoadSpec prefers source-embedded specs when the // LITTEST marker is present.
Consider:
- Converting to
{{.*}}wildcards for consistency - Deleting
cl/_testdata/embedunexport/out.ll
| // CHECK-LABEL: define %"github.com/goplus/llgo/runtime/internal/runtime.eface" @"{{.*}}.Bar"() { | ||
| // CHECK: ret %"github.com/goplus/llgo/runtime/internal/runtime.eface" |
There was a problem hiding this comment.
Nit: These two functions use the fully qualified %"github.com/goplus/llgo/runtime/internal/runtime.eface" type, while all other type references in this file (and across the migration) use {{.*}} wildcards (e.g., %"{{.*}}.Foo"). Consider using %"{{.*}}.eface" here for consistency.
| // CHECK-LABEL: define { i64, %"{{.*}}.iface" } @"{{.*}}.FuncWithAllTypeParams"({{.*}}) !dbg !{{[0-9]+}} { | ||
| // CHECK: %"{{.*}}.Slice" @"{{.*}}.NewSlice3"(ptr %54, i64 8, i64 3, i64 0, i64 3, i64 3) | ||
| // CHECK: call void @"{{.*}}.PrintComplex"( | ||
| // CHECK: call void @"{{.*}}.PrintSlice"( | ||
| // CHECK: call void @"{{.*}}.PrintIface"(%"{{.*}}.iface" | ||
| // CHECK: call %"{{.*}}.iface" @errors.New( | ||
| // CHECK: ret { i64, %"{{.*}}.iface" } | ||
| // CHECK-LABEL: define void @"{{.*}}.FuncWithAllTypeStructParam"({{.*}}) !dbg !{{[0-9]+}} { | ||
| // CHECK: %3 = call ptr @"{{.*}}.AllocZ"(i64 288) |
There was a problem hiding this comment.
These CHECK-LABEL blocks verify FuncWithAllTypeParams (defined at ~line 95) and FuncWithAllTypeStructParam (defined at ~line 61), but are placed here before the unrelated ScopeIf function. This is presumably because the IR emission order differs from source order.
Consider adding a brief comment explaining this, e.g.:
// The following CHECK blocks verify functions defined earlier whose IR is emitted after FuncStructPtrParams.This would help future readers understand why the assertions appear detached from their corresponding source.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1780 +/- ##
=======================================
Coverage 88.44% 88.44%
=======================================
Files 50 50
Lines 13656 13656
=======================================
Hits 12078 12078
Misses 1369 1369
Partials 209 209 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4b85b72 to
ba756e2
Compare
Signed-off-by: ZhouGuangyuan <zhouguangyuan.xian@gmail.com>
ba756e2 to
b3c001a
Compare
Summary
Testing