cl: basic framework - #701
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: cl package skeleton + clang method renames
This PR lays down the C-to-Go compiler front-end skeleton (compileDecl/compileFunc, blockCtx, type/var helpers, a test helper) and mechanically renames many clang.Cursor methods in lib/clang/clang.go. The structure follows the established XGo/llcppg conventions and builds/vets cleanly. Most panic("todo") and commented-out blocks are expected WIP scaffolding.
Verified as correct (not flagged):
- The
lib/clang/clang.gorenames are consistent: every reordered doc comment stayed matched to the correct// llgo:link C.clang_*target and function body. The double-space comment typo fixes are also fine. for i := range nwithnewParam(i c.Int)is correct under Go 1.27 (range-over-int yields the operand's typec.Int), andDbgFlagAllcomputes to 3 as intended.
A few noteworthy items are left as inline comments below. The most important is the void-return handling (already TODO-flagged) and the inaccurate Config.Fset doc comment.
| pkg := ctx.pkg | ||
| retType := fn.ResultType() // TODO(xsw): return void | ||
| tyRet := toType(ctx, retType, flagRetType) | ||
| results = types.NewTuple(pkg.NewParam(token.NoPos, "", tyRet, false)) |
There was a problem hiding this comment.
Void return produces an invalid signature. results is unconditionally built as a single unnamed result: for a C function returning void, toType will yield a void tyRet and this generates a Go func with one unnamed result rather than no results. The author already flagged this with // TODO(xsw): return void (line 236); worth resolving before toType is implemented, since void-returning functions are common and would otherwise emit broken output.
| // Config specifies the configuration for compiling C/C++ header files. | ||
| type Config struct { | ||
| // Fset provides source position information for syntax trees and types. | ||
| // If Fset is nil, Load will use a new fileset, but preserve Fset's value. |
There was a problem hiding this comment.
This doc comment is inaccurate and self-contradictory: there is no Load function in this package (the entry point is NewPackage), and "If Fset is nil ... preserve Fset's value" cannot be true when Fset is nil. NewPackage passes conf.Fset to gogen and never writes it back. Suggest: // If Fset is nil, NewPackage will create a new fileset.
| origName := fnName | ||
| rewritten := ctx.getPubName(&fnName) | ||
| n := fn.NumArguments() | ||
| var params []*types.Var |
There was a problem hiding this comment.
The exact argument count is already known from n := fn.NumArguments(), so the append loop below reallocates the backing array as it grows. Preallocate to avoid the copies:
params := make([]*types.Var, 0, n+1) // +1 for the optional variadic param| sig := types.NewSignatureType(nil, nil, nil, types.NewTuple(params...), results, variadic) | ||
| f := types.NewFunc(ctx.goNodePos(fn), pkg.Types, fnName, sig) | ||
| if old := pkg.Types.Scope().Insert(f); old != nil { | ||
| log.Panicln("Go func", fnName, "redefined") |
There was a problem hiding this comment.
NewPackage/loadFile carry an err error return, but this path (and compileDecl's log.Panicln("compileDecl: unknown kind ...") at line 211, and substObj) crash the process instead of propagating errors. Duplicate symbols and unhandled declaration kinds are input-controlled, so any header with a currently-unhandled top-level kind aborts. Consider deciding the error strategy — propagate err vs. panic-and-recover at the top level — since the public API advertises error.
| continue | ||
| } | ||
| t.Run(name, func(t *testing.T) { | ||
| pkgDir := dir + "/" + name |
There was a problem hiding this comment.
Minor: dir was built with path.Join above, but this joins manually. Prefer pkgDir := path.Join(dir, name) for consistency.
1 participant
No description provided.