Repository navigation
cl: conf.MethodCheck; compileFuncOrMethod: call ctx.methodCheck; NewPackage: compilation failed with errors; tool test; TestPython support MethodCheck; - #946
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: MethodCheck — Logical-Type Method Detection
Clean, focused feature. Backward compatibility is well preserved (no MethodCheck → methodCheck returns ("", true), keeping existing C method-detection behavior). The matchResult buffer sizing (len(result)+n) is a safe upper bound that never reallocates, and the make([]string, ...) in check is correctly gated behind the CutPrefix early-exit so the common non-matching path allocates nothing.
Main concern: degenerate / malformed MethodCheck config entries are accepted silently and produce invalid class names rather than failing fast. See inline notes. No security-severity or performance regressions found.
Not inline:
- The
MethodCheckDSL (*wildcard capture → positional$1..$9substitution,prefix:resultform, empty result = "not a method") is only documented in the referenced issue. Consider a doc comment onnewMethodCheck/matchResultor a README note so operators can learn the syntax without opening the issue.tool/config.go:42's comment "C/C++ method check list" is also uninformative compared to the richerConfig.MethodCheckdoc.
| func matchResult(result string, match []string, n int) string { | ||
| b := make([]byte, 0, len(result)+n) | ||
| for i := 0; i < len(result); i++ { | ||
| if result[i] == '$' { | ||
| if i+1 < len(result) { | ||
| i++ | ||
| c := result[i] | ||
| if c >= '1' && c <= '9' { | ||
| if index := int(c - '1'); index < len(match) { | ||
| b = append(b, match[index]...) | ||
| continue | ||
| } | ||
| } | ||
| b = append(b, '$', c) | ||
| continue | ||
| } | ||
| } | ||
| b = append(b, result[i]) | ||
| } | ||
| return string(b) |
There was a problem hiding this comment.
[P2] matchResult silently emits literal $N for out-of-range captures
matchResult passes an out-of-range $N reference through as a literal (e.g. $1 with zero capture groups appends the literal string "$1"). Combined with ok = cls != "", this returns cls="$1", ok=true — a class name that is not a valid Go identifier, producing a broken binding downstream instead of a clear error. A config like "Py_:$1" (no *) triggers this. Consider validating at parse time in newMethodCheck that every $N in result references an existing capture group.
| func newMethodCheck(check string) (*mthdCheck, error) { | ||
| pos := strings.IndexByte(check, ':') | ||
| if pos < 0 { | ||
| return nil, fmt.Errorf("invalid MethodCheck: %s", check) | ||
| } | ||
| pattern := strings.TrimSpace(check[:pos]) | ||
| parts := strings.Split(pattern, "*") | ||
| result := strings.TrimSpace(check[pos+1:]) | ||
| return &mthdCheck{ | ||
| prefix: parts[0], | ||
| pattern: parts[1:], | ||
| result: result, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
[P2] Degenerate/empty patterns match everything, no validation
newMethodCheck performs no validation beyond requiring a :. An entry whose prefix is empty (e.g. ":Foo") or that contains adjacent/leading/trailing * (producing empty pattern parts) matches every function name: strings.CutPrefix(name, "") always succeeds and strings.Index(name, "") returns 0. The result is that the static result overrides the inferred typName for all functions, which is almost certainly unintended. Recommend rejecting empty prefix/pattern parts in newMethodCheck so a bad config fails fast.
| } | ||
| n := 0 | ||
| match := make([]string, len(p.pattern)) | ||
| for i, p := range p.pattern { |
There was a problem hiding this comment.
[P3] Range variable p shadows the *mthdCheck receiver in check()
In check(), the range variable p shadows the method receiver p *mthdCheck. It compiles today only because p.result is read outside the loop and the string p is used inside it. This is a maintenance landmine: any future edit referencing a receiver field (p.prefix, p.result) inside the loop body would silently bind to the string element. Rename the range variable (e.g. pat).
1 participant
MethodCheck— Type Method Detection in llcppg #945