Skip to content

Conversation

@Vipul-Cariappa
Copy link
Collaborator

Description

adapted from cling

Fixes # (issue)

Fixes 2 issues in cppyy.

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Requires documentation updates

Testing

Included.

Checklist

  • I have read the contribution guide recently

Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

LGTM!

@codecov
Copy link

codecov bot commented Jun 20, 2025

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.77%. Comparing base (130dd30) to head (c75106a).
⚠️ Report is 58 commits behind head on main.

Files with missing lines Patch % Lines
lib/CppInterOp/CppInterOp.cpp 93.75% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #640      +/-   ##
==========================================
+ Coverage   78.69%   78.77%   +0.07%     
==========================================
  Files           9        9              
  Lines        3835     3849      +14     
==========================================
+ Hits         3018     3032      +14     
  Misses        817      817              
Files with missing lines Coverage Δ
lib/CppInterOp/CppInterOp.cpp 86.67% <93.75%> (+0.08%) ⬆️
Files with missing lines Coverage Δ
lib/CppInterOp/CppInterOp.cpp 86.67% <93.75%> (+0.08%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

SourceLocation noLoc;
QualType TT = S.CheckTemplateIdType(TemplateName(TemplateD), noLoc, TLI);
if (TT.isNull())
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd drop that if we can't cover it with tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is required; it is due to a template instantiation failing in Eigen (crashes).
Also, fixing the instantiation in this PR does not make sense. Will look into that once I get to the eigen tests.
And writing a test that is supposed to fail is bad again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Understood

Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s merge it then.

@vgvassilev vgvassilev merged commit bc728d3 into compiler-research:main Jun 21, 2025
50 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.

2 participants