Repository navigation
cl: compileFuncOrMethod creatorCheck use ctx.cstyleToGo; mod: github.com/llarhub/clang-c v0.9.0 - #962
Conversation
xushiwei
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #962 +/- ##
=======================================
Coverage 88.08% 88.09%
=======================================
Files 23 23
Lines 2686 2687 +1
=======================================
+ Hits 2366 2367 +1
Misses 320 320
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: PR #962 — creatorCheck normalizes creator name via cstyleToGo
Summary. This PR adds one line in cl/func.go that routes the config-derived creator token through ctx.cstyleToGo(...) before it becomes the generated Go function/method name, plus a clang-c dependency bump (v0.8.0 → v0.9.0). The change is correct and safe to merge: it makes the creator path consistent with every other name-producing path in the tool (globalName, localName, varName, typeName, funcName all end in cstyleToGo), which previously was the lone path emitting raw rule text. For the existing Python test corpus the $N captures are already PascalCase, so cstyleToGo is a no-op and no golden output changes — verified cl builds cleanly against clang-c v0.9.0.
Verified:
go build ./cl/...succeeds;clang-c v0.9.0resolves and downloads.- Dependency hashes checked against the Go checksum database: both the module-zip hash and the
go.modhash forv0.9.0matchsum.golang.org. The unchangedgo.modhash acrossv0.8.0→v0.9.0is expected (module metadata didn't change) and benign — the zip hash still changed and would catch tampering. - No security concern:
creatororiginates from the developer-authoredNewCheckconfig (a local codegen trust model); normalization reduces the chance of malformed content landing in generated source, and a bad name produces non-compiling Go, not injection. - No performance concern: one bounded per-function string op on a codegen path.
The findings below are minor/consistency-level and non-blocking.
Additional findings
tool/config.go:42: [P3] NewCheck doc now understates behavior (creator names are normalized):NewCheckis documented only as "Type Creator Detection (optional). See .../issues/960." With this PR, the creator token in a rule (e.g..As$1) is now run throughcstyleToGo, so a token containing lowercase-initial segments, underscores, or::is capitalized/camel-cased rather than emitted verbatim — a user-visible contract shift for rule authors not reflected in any comment or doc. Optional: add a one-line clarification here (e.g. "the creator name in the rule is normalized to a Go-style identifier, same conversion as other generated names") to keep the referenced documentation accurate.
| if creator != "" { | ||
| if newResults, ok := tryNewResults(pkgTypes, obj, results); ok { | ||
| results = newResults | ||
| creator = ctx.cstyleToGo(creator, false, true) |
There was a problem hiding this comment.
[P3] Co-locate creator normalization in name.go with creatorCheck
Every other Go-name derivation co-locates cstyleToGo with its derivation in cl/name.go (globalName, localName, varName, typeName, funcName all return a ready-to-use identifier). Here the token is derived in creatorCheck (name.go) but normalized in func.go, so the caller must remember to normalize. Consider moving this cstyleToGo call into creatorCheck (or a small creatorName helper) so it returns an already-normalized creator, matching the established pattern and keeping func.go focused on compilation logic. Non-blocking — current placement is functionally correct (it sits after results = newResults and before both fnName = creator consumers, and only runs when tryNewResults succeeds).
| if creator != "" { | ||
| if newResults, ok := tryNewResults(pkgTypes, obj, results); ok { | ||
| results = newResults | ||
| creator = ctx.cstyleToGo(creator, false, true) |
There was a problem hiding this comment.
[P3] underscoreStart hardcoded to false diverges from other callers
Every other cstyleToGo caller computes underscoreStart (via rmPrefixAndUnderscoreStart / checkUnderscoreStart) so a leading _ is preserved through goNameOf. This call pins it to false. For the intended use (an exported creator such as AsByteArray) false is correct, but if a $N wildcard expansion ever yields a creator starting with _, that underscore would be silently dropped unlike every other name path. Low probability given creators are author-controlled exported names. If false is intentional, a one-line comment stating creators are always treated as exported, non-underscore names would prevent a future reader from assuming it's a copy-paste oversight.
1 participant
No description provided.