Skip to content

cl: ctx.logf/panicf - #873

Merged
xushiwei merged 4 commits into
goplus:mainfrom
xushiwei:q
Sep 29, 2026
Merged

xushiwei merged 4 commits into
goplus:mainfrom
xushiwei:q

Conversation

@xushiwei

Copy link
Copy Markdown
Member

No description provided.

@codecov

codecov Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.22222% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.00%. Comparing base (bf357bc) to head (bed9e2c).
⚠️ Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
cl/ctx.go 71.42% 2 Missing ⚠️
cl/class.go 83.33% 1 Missing ⚠️
cl/func.go 75.00% 1 Missing ⚠️
cl/var.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #873      +/-   ##
==========================================
+ Coverage   85.33%   86.00%   +0.66%     
==========================================
  Files          22       22              
  Lines        1821     1822       +1     
==========================================
+ Hits         1554     1567      +13     
+ Misses        267      255      -12     
Flag Coverage Δ
llgo-tests 86.00% <72.22%> (+0.66%) ⬆️

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.

@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.

Code Review — PR #873

Nice cleanup: routing skip/error diagnostics through pkgCtx.logf/panicf so they carry file:line positions is a clear improvement over bare log.Println, and adding clang.USR gives funcUSR a more reliable key than the display name. The Doxygen /** */ → // conversion in clang/clang.go and enabling TestLLVM_DenseMapInfo are safe.

A few items worth addressing, flagged inline:

  • panicf can nil-deref — goNode returns nil for InvalidFile cursors, but panicf calls node.Pos()/node.End() without a nil check (its sibling logf is safe via goNodePos). Reachable through the loadClassMember default branch.
  • Two misleading skip messages — the obj.order() >= 0 overload case reuses the "unsupported template params" text, and the empty-mangling case reuses "with template arguments".
  • Leftover Doxygen \c artifact in a converted comment.

Design note (not inline): several skip messages that were previously gated behind debugCompileDecl are now emitted unconditionally via ctx.logf. Each fires per skipped declaration and does real work (libclang Extent/Spelling cgo calls, fset.Position lookup, clang.String(...) allocation, and an unconditional stderr write). For large headers where many decls are skipped (symbol-not-found, template args, unsupported features), this changes default output verbosity and adds per-decl overhead that was previously zero. If surfacing skip reasons by default is intentional, this is fine — otherwise consider keeping them behind the debug flag. Flagging so it's a conscious choice.

Comment thread cl/ctx.go Outdated
Comment thread cl/class.go
Comment thread cl/func.go Outdated
Comment thread clang/clang.go
@xushiwei
xushiwei merged commit de8a20a into goplus:main Sep 29, 2026
3 of 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