cl: compileFunc; clang: FileContents - #705
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: PR #705
This PR wires up the initial function-declaration compile path: a token-file/position layer (node, nodeInterp, goNodePos, blockCtx.initFile), new libclang bindings (TranslationUnit.File/FileContents, File alias), the Source.Handle field, and small toType/compileFunc fixes. Early-stage scaffolding with intentional TODOs; structure is clean. Findings below are robustness/clarity notes on the newly added/changed lines.
Notable non-inline observations
cl/compile.go:132—_ = confdiscardsConfig(includingNameLookup/Include) that the test atcompile_test.gosets. A brief// TODOnoting these aren't wired up yet would clarify intent.- The
defaultpanics incompileDecl(compile.go) andtoTypemean parsing only works for a narrow set of cursor/type kinds today; expected for WIP, but worth tracking before real headers are parsed.
| func (u TranslationUnit) FileContents(file File) []byte { | ||
| var size c.SizeT | ||
| data := u.impl.FileContents(file, &size) | ||
| return unsafe.Slice((*byte)(unsafe.Pointer(data)), int(size)) |
There was a problem hiding this comment.
[P1] FileContents: guard nil pointer and unsigned->int size narrowing
clang_getFileContents returns NULL (with size 0) when the file is not loaded/buffered in the TU. unsafe.Slice on a nil pointer with a non-zero/garbage size yields an invalid slice that panics on access. Also, int(size) narrows an unsigned c.SizeT to signed int, which can go negative/truncated for a pathologically large or bogus size. Recommend returning nil when data == nil before constructing the slice.
Note also that the returned []byte aliases C-owned memory tied to the TranslationUnit lifetime (invalid after Dispose()), and the doc comment states unconditionally that it "returns the contents" — worth documenting the empty/nil case and the aliasing/lifetime contract for future callers.
| 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") | ||
| f, err := pkg.NewFuncWith(goNodePos(ctx, fn), fnName, sig, nil) |
There was a problem hiding this comment.
[P2] compileFunc: redeclaration will panic on repeated C decls
This calls pkg.NewFuncWith unconditionally and log.Paniclns on error. C headers routinely declare the same function multiple times (via includes). The commented-out reference implementation guarded with checkExists. Please confirm NewFuncWith tolerates duplicates given SetRedeclarable(true); if not, this will crash on realistic input. A dedup/exists guard may be needed.
| type Source struct { | ||
| TU clang.TranslationUnit | ||
| Handle clang.File | ||
| PresumedFile *c.Char |
There was a problem hiding this comment.
[P3] Source.PresumedFile: unused, undocumented exported field
PresumedFile *c.Char is an exported field that is never populated or read anywhere in the repo. Either add a doc comment describing its intended use, or drop it to avoid an undocumented public API surface. Raw *c.Char values (e.g. from a clang.String) also carry a use-after-free footgun once their source is Dispose()d, so an ownership note would help if it's kept.
| default: | ||
| log.Println("==> toType: unknown Kind -", typ.Kind) |
There was a problem hiding this comment.
[P3] toType: default branch logs then falls through to panic
The new default case log.Printlns the unknown kind and then execution falls through to the panic("todo: toType ...") at the end of the function, producing duplicate diagnostics for the same event. Consider panicing directly in the default case (with the kind) instead of logging then panicking. The flags argument is also still unused in the switch — a // TODO would clarify that's intentional.
1 participant
No description provided.