Skip to content

Place CPython thread types in their defining header’s Go file - #925

Merged
xushiwei merged 5 commits into
mainfrom
copilot/fix-tool-testpython-bugfix
Oct 5, 2026
Merged

xushiwei merged 5 commits into
mainfrom
copilot/fix-tool-testpython-bugfix

Conversation

Copilot AI commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

TestPython passed despite placing X_PyTssT in pythread.go instead of cpython-pythread.go.

  • File routing: Route forward class and struct declarations to the header containing their definition.
  • Python goldens: Move X_PyTssT into cpython-pythread.go; keep its PyTssT alias in pythread.go.
// cpython-pythread.go
type X_PyTssT struct { ... }

// pythread.go
type PyTssT = X_PyTssT

Copilot AI linked an issue Oct 5, 2026 that may be closed by this pull request
Copilot AI and others added 2 commits October 5, 2026 08:19
Co-authored-by: xushiwei <396972+xushiwei@users.noreply.github.com>
Co-authored-by: xushiwei <396972+xushiwei@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix TestPython bug output in tool package Place CPython thread types in their defining header’s Go file Oct 5, 2026
Copilot AI requested a review from xushiwei October 5, 2026 08:21
@xushiwei
xushiwei marked this pull request as ready for review October 5, 2026 08:22

@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

The core fix in cl/compile.go is small, well-targeted, and follows the existing idioms in the cl/ package (IsCursorDefinition() == 0, Definition(), IsNull() == 0, clang.PresumedFile). For a forward-declared class/struct it resolves the file/package from the definition's location instead of the forward declaration's location, which correctly fixes the misattribution the generated fixture change (X_PyTssT moving from pythread.go to cpython-pythread.go) demonstrates. The null-definition guard is correct and the change is behaviorally conservative. Build passes.

No security concerns. The go.mod/go.sum pruning in tool/_testcpp/llvm-22.1.8-support/ leaves a consistent module graph (removed llarhub/libcxx and llarhub/llvm-c are no longer referenced by any Go import there). Generated fixtures are mechanical and out of scope.

Findings below are non-blocking; the main one worth a decision is whether enum/union forward declarations should get the same treatment.

Comment thread cl/compile.go
Comment on lines +290 to +291
switch decl.Kind {
case lc.Cursor_ClassDecl, lc.Cursor_StructDecl:

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.

[P2] Same forward-decl misattribution likely applies to enum/union

This switch only special-cases Cursor_ClassDecl/Cursor_StructDecl. Enum and union declarations short-circuit on the exact same condition elsewhere (enum.go:65 and union.go:67 both do if decl.IsCursorDefinition() == 0 { return }), so an enum/union forward-declared in file A and defined in file B can be attributed to the wrong file by the same mechanism this PR fixes for class/struct. Enum forward declarations with a fixed underlying type (enum E : int;) are legal and common C++11.

If the broader fix is intended, consider also handling lc.Cursor_EnumDecl (and lc.Cursor_UnionDecl). If class/struct is the deliberate scope, a short comment explaining why would prevent a future reader from assuming it was an oversight. (Typedefs are correctly excluded — they are not a forward/definition pair.)

Comment thread cl/compile.go
@xushiwei

xushiwei commented Oct 5, 2026

Copy link
Copy Markdown
Member

@copilot Fix above review comments. And revert any changes in tool/_testcpp/llvm-22.1.8-support (it makes some other test cases failed).

Copilot AI and others added 2 commits October 5, 2026 08:43
Co-authored-by: xushiwei <396972+xushiwei@users.noreply.github.com>
Co-authored-by: xushiwei <396972+xushiwei@users.noreply.github.com>

Copilot AI commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Fix above review comments. And revert any changes in tool/_testcpp/llvm-22.1.8-support (it makes some other test cases failed).

Restored the LLVM fixture dependency metadata; the affected TestLLVM_* cases now pass. The fix is in eea6704. I also removed toolchain-installer changes to the root module files in 9b67a2e.

@xushiwei
xushiwei merged commit 4d16c90 into main Oct 5, 2026
2 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.

fix(tool): TestPython bugfix

2 participants