cl: TestMockC ok - #706
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: PR #706
This PR bumps github.com/goplus/gogen v1.23.5 → v1.24.0, enables the previously-disabled mock-C test (_TestMockC → TestMockC) with a new cl/_testmockc/function/ fixture, and adds a small standalone cmd/llgogen demo.
Verified locally:
go build ./cmd/llgogen/succeeds;go run ./cmd/llgogen/outputs the expected empty-bodyfunc g().go.sumhashes for gogen v1.24.0 are consistent and legitimate (checked against the Go checksum DB by the security pass).TestMockCitself requires the llgo/clang toolchain and can't be run in this plain-go testsandbox (relocation targets likeC.clang_createIndexundefined) — build failure is an environment limitation, not a PR defect. Thefunctionfixture covers unsigned-return, void-no-arg, and variadic (__llgo_va_list ...any) cases, which is good coverage.
Overall this is a clean, low-risk change. Two minor/optional notes below; nothing blocking.
|
|
||
| func _TestMockC(t *testing.T) { | ||
| func TestMockC(t *testing.T) { | ||
| cl.SetDebug(cl.DbgFlagAll) |
There was a problem hiding this comment.
Now that TestMockC is enabled and run on every go test, the unconditional cl.SetDebug(cl.DbgFlagAll) will emit debug log.Println output (from compileFunc / param handling in cl/compile.go) on every run, adding CI noise with no assertion value — the golden-file diff is what asserts correctness.
Also note SetDebug mutates package-level globals (debugCompileDecl/debugLoadDeps) and is never reset. It's harmless today because TestMockC is the only test in cl_test, but if another test is later added to this package it would silently inherit DbgFlagAll depending on run order. Consider dropping the call, gating it behind testing.Verbose(), and/or defer cl.SetDebug(0).
| pkg := gogen.NewPackage("", "foo", &gogen.Config{ | ||
| Importer: packages.NewImporter(nil), | ||
| LoadNamed: nil, | ||
| HandleErr: nil, |
There was a problem hiding this comment.
Nit: these six fields (LoadNamed, HandleErr, NewBuiltin, NodeInterpreter, CanImplicitCast, DefaultGoFile) are all set to their zero values, so they're no-ops. Trimming the literal to just Importer: packages.NewImporter(nil) makes the meaningful config clearer and avoids a needless compile break if gogen renames/removes any of these fields in a future bump. Optional.
1 participant
No description provided.