Skip to content

Add type info interfaces motivated by numba #534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aaronj0
Copy link
Collaborator

@aaronj0 aaronj0 commented Mar 20, 2025

  • Add IsSameType used for matching arg types between numba inferred signatures and CppOverloads in cppyy.
  • Add type info reflection interfaces for integer, void pointer, signed and unsigned types.
  • Also added interfaces IsIntegral and IsFloating that check if types have the respective representations. Used in the case of Numba where scoring based on reflection is required.

will add tests

- Add `IsSameType` used for matching arg types between numba inferred signatures and CppOverloads in cppyy.
- Add type info reflection interfaces for integer, void pointer, signed and unsigned types.
- Also added interfaces `IsIntegral` and `IsFloating` that check if types have the respective representations. Used in the case of Numba where scoring based on reflection is required.
@aaronj0 aaronj0 requested a review from vgvassilev March 20, 2025 23:26
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -1573,8 +1609,17 @@ namespace Cpp {
return QT.getNonReferenceType().getAsOpaquePtr();
}

TCppType_t GetUnqualifiedType(TCppType_t type) {
if (!type)
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use nullptr [modernize-use-nullptr]

Suggested change
return 0;
return nullptr;

TCppType_t GetUnderlyingType(TCppType_t type)
{
if (!type)
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use nullptr [modernize-use-nullptr]

Suggested change
return 0;
return nullptr;

Copy link

codecov bot commented Mar 20, 2025

Codecov Report

Attention: Patch coverage is 3.44828% with 28 lines in your changes missing coverage. Please review.

Project coverage is 72.16%. Comparing base (f0f59f2) to head (21266fe).

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

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #534      +/-   ##
==========================================
- Coverage   72.71%   72.16%   -0.56%     
==========================================
  Files           9        9              
  Lines        3618     3647      +29     
==========================================
+ Hits         2631     2632       +1     
- Misses        987     1015      +28     
Files with missing lines Coverage Δ
include/clang/Interpreter/CppInterOp.h 96.29% <ø> (ø)
lib/Interpreter/CppInterOp.cpp 81.84% <3.44%> (-1.17%) ⬇️
Files with missing lines Coverage Δ
include/clang/Interpreter/CppInterOp.h 96.29% <ø> (ø)
lib/Interpreter/CppInterOp.cpp 81.84% <3.44%> (-1.17%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vgvassilev
Copy link
Contributor

We should add tests.

@jeaye
Copy link
Contributor

jeaye commented Apr 25, 2025

Would love to have this. What's needed before we can merge?

@aaronj0
Copy link
Collaborator Author

aaronj0 commented Apr 25, 2025

Would love to have this. What's needed before we can merge?

Hi! I have some unittests that I haven't pushed yet, and this PR should be ready to go. Will do in the next few hours once I have access to a laptop

@@ -509,15 +509,40 @@ namespace Cpp {
/// Checks if the provided parameter is a Plain Old Data Type (POD).
CPPINTEROP_API bool IsPODType(TCppType_t type);

/// Checks if type has an integer representation
CPPINTEROP_API bool IsIntegerType(TCppType_t type);
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we should have an enum instead of many interfaces..

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

bool IsSignedIntegerType(TCppType_t type) {
QualType QT = QualType::getFromOpaquePtr(type);
return QT->hasSignedIntegerRepresentation();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

    auto *D = (Decl *) var;
              ^

return QT->hasSignedIntegerRepresentation();
}

bool IsUnsignedIntegerType(TCppType_t type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: redundant boolean literal in conditional return statement [readability-simplify-boolean-expr]

lib/Interpreter/CppInterOp.cpp:1565:

-     if (llvm::isa_and_nonnull<VarDecl>(D)) {
-       return true;
-     }
- 
-     return false;
+     return llvm::isa_and_nonnull<VarDecl>(D);

bool IsFloatingType(TCppType_t type) {
QualType QT = QualType::getFromOpaquePtr(type);
return QT->hasFloatingRepresentation();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]

    auto *D = (clang::Decl *) var;
              ^

std::vector<Decl*> Decls;
std::string code = R"(
int a;
int *b;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function 'TEST' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]

Suggested change
int *b;
TESTstatic (TypeReflectionTest, IntegerTypes) {

std::vector<Decl*> Decls;
std::string code = R"(
class A {};
using VoidPtrType = void*;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function 'TEST' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]

Suggested change
using VoidPtrType = void*;
TESTstatic (TypeReflectionTest, VoidPtrType) {

std::vector<Decl*> Decls;

std::string code = R"(
#include <cstdarg>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function 'TEST' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]

Suggested change
#include <cstdarg>
TESTstatic (TypeReflectionTest, IsSameType) {


Decls.assign(Decls.end() - 8, Decls.end());

EXPECT_TRUE(Cpp::IsSameType(Cpp::GetType("bool"), Ctxt.BoolTy.getAsOpaquePtr()));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: included header iostream is not used directly [misc-include-cleaner]

Suggested change
EXPECT_TRUE(Cpp::IsSameType(Cpp::GetType("bool"), Ctxt.BoolTy.getAsOpaquePtr()));

std::vector<Decl*> Decls;
std::string code = R"(
class A {};
int a;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function 'TEST' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]

Suggested change
int a;
TESTstatic (VariableReflectionTest, Is_Get_Reference) {

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.

3 participants