Skip to content

cl: compileFuncOrMethod bugfix - ctx.methodCheck should before tryToMethod - #954

Merged
xushiwei merged 1 commit into
goplus:mainfrom
xushiwei:lcls
Oct 6, 2026
Merged

xushiwei merged 1 commit into
goplus:mainfrom
xushiwei:lcls

Conversation

@xushiwei

@xushiwei xushiwei commented Oct 6, 2026

Copy link
Copy Markdown
Member

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

This PR restructures the C-global-function-to-method conversion in compileFuncOrMethod so that methodCheck(cName) now gates whether tryToMethod runs at all, rather than only being consulted inside the isPtr branch for logical-class remapping. The regenerated pythread.go testdata reflects the new output (Py_IsTrue -> free IsTrue, PyObject_IsTrue -> (*Object).IsTrue).

Assessment: The restructuring is correct and the control flow is sound. methodCheck returns ("", true) when no MethodCheck patterns are configured, so the default path is unchanged. The isPtr && cls != "" && cls != typName guard correctly folds the former nested conditions with identical semantics, and fn.Argument(0) remains safe since it is only reached when typRecv != nil. No correctness, performance, security, or documentation-accuracy defects were found. Two minor items below.

Comment thread cl/func.go

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review (follow-up)

One additional note that could not be attached in the prior review event.

Comment thread cl/func.go
@codecov

codecov Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.98%. Comparing base (137b76f) to head (c532660).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #954      +/-   ##
==========================================
- Coverage   87.99%   87.98%   -0.01%     
==========================================
  Files          23       23              
  Lines        2665     2664       -1     
==========================================
- Hits         2345     2344       -1     
  Misses        320      320              
Flag Coverage Δ
llgo-tests 87.98% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@xushiwei
xushiwei merged commit a22ed14 into goplus:main Oct 6, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant