cl name refactor: use cName instead of origName - #890
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #890 +/- ##
==========================================
+ Coverage 87.64% 87.71% +0.06%
==========================================
Files 23 23
Lines 1951 1962 +11
==========================================
+ Hits 1710 1721 +11
Misses 241 241
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
This PR refactors naming to derive C/C++ full names (cName) directly from the AST cursor (new helpers cNameOf/cNS/cBaseName/cNameSplit/cNameWithNS/cTypeName) instead of threading an ns string through the load/compile paths, and adds an NSPrefix config option. The refactor is coherent and consistently applied, the NSPrefix wiring is complete end-to-end (config → gen → compile → ctx → name), and the cstyleToGo rewrite preserves prior behavior for the leading-underscore case.
Findings below are minor — a leftover dead function, a log-format typo, and doc comments that still describe the removed ns parameter. None are blocking. One robustness note (latent hang in cNameOf/cNS) is included for awareness.
Not blocking / verified correct: the inlined cstyleToGo leading-_ handling (old cPubName's _→X branch was unreachable from cstyleToGo since _ is a split char), the new rename[part] v != "" guard (a safe improvement), and the lastEndWithUpper separator logic. Performance of the new per-reference AST walks is negligible for a one-shot code generator.
Findings not placed inline
cl/name.go:273—nameWithNSis now dead code. After this refactor its only remaining reference is its own definition; all former callers moved to the newcNameWithNS/cstyleToGoscheme. Note it behaves differently from the similarly-namedcNameWithNS(nsName-based vs"::"-joined), so keeping both invites future misuse. Recommend deleting it.tool/config.go:53—NSIgnoredoc is now stale. The struct tag comment still reads// namespaces (Go style names) to ignore, butisNSIgnorednow matches against C/C++ names (loadNamespacecomputescNameOf(namespace)). The parallel comment incl/compile.gowas correctly updated to "C/C++ namespaces"; this one was missed and now contradicts it. (Line not in the diff, so noted here.)cl/name.gocNameOf/cNS— latent infinite loop on a null/invalid cursor. Both loops terminate only onCursor_TranslationUnit; libclang's semantic parent of a null cursor is itself null (Cursor_InvalidFile), so an invalid cursor would loop forever. Current call sites always pass valid cursors, so this is defensive only — the rest of the codebase guards withIsNull()(e.g.cl/vtable.go,cl/class.go). Consider adding anif decl.IsNull() != 0 { break }guard.
| underlying := decl.TypedefDeclUnderlyingType() | ||
| if debugCompileDecl { | ||
| log.Println("typedef", origName, "-", clang.String(underlying)) | ||
| ctx.logf(decl, "typedef %ss: %s", cName, clang.String(underlying)) |
There was a problem hiding this comment.
Log format typo: "typedef %ss: %s" renders as e.g. typedef Foos: ... (double s). Every other log in this file uses "typedef %s: ..." (lines 31, 37, 51, 56). Should be "typedef %s: %s". Only emitted under debugCompileDecl.
| name := clang.String(decl) | ||
| origName := nameWithNS(name, ns) | ||
| anonymous := decl.IsAnonymous() != 0 | ||
| func loadEnum(ctx *pkgCtx, decl clang.Cursor) { |
There was a problem hiding this comment.
Stale doc comment above this signature still describes the removed ns parameter: "ns carries the enclosing namespace/class prefix (e.g. \"bar_\" or \"Shape_\") ... through getPubName". loadEnum no longer takes ns — it derives ns := cNS(decl) internally, which is a ::-joined C++ namespace (e.g. bar, Shape), not an underscore-suffixed Go prefix, and there is no getPubName. Please update the comment.
| func loadUnion(ctx *pkgCtx, decl clang.Cursor, ns string) { | ||
| if decl.IsCursorDefinition() == 0 { | ||
| return | ||
| func loadUnion(ctx *pkgCtx, decl clang.Cursor) { |
There was a problem hiding this comment.
Stale doc comment above this signature: "ns carries the enclosing prefix ... through getPubName like a struct's". loadUnion no longer takes an ns parameter (it now computes cName := cNameOf(decl)), and getPubName no longer exists. Please update.
| // handled). Non-static class member variables are fields, not VarDecls, and are | ||
| // handled separately in loadClassMember. | ||
| func loadVar(ctx *pkgCtx, decl clang.Cursor, ns string) { | ||
| func loadVar(ctx *pkgCtx, decl clang.Cursor) { |
There was a problem hiding this comment.
Stale doc comment above this signature: "The Go name is prefixed by ns, which encodes the enclosing namespaces and/or class". loadVar/compileVar no longer take ns; the name is now derived via cNameOf(decl). Please update the comment to match.
1 participant
cName := full C/C++ name (eg. foo::bar::f)
origName := concat parts (eg. FooBarF)