Skip to content

Resolve static attributes of records #322

New issue

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

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

Already on GitHub? # to your account

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

Vipul-Cariappa
Copy link
Collaborator

Fixes: test04_class_read_access and test05_class_data_write_access with one-liner change in cppyy.

@Vipul-Cariappa
Copy link
Collaborator Author

@vgvassilev, Should this change be extracted to a separate function GetStaticDatamembers? If we extract it to a separate function we will have to modify CPyCppyy slightly.

@aaronj0
Copy link
Collaborator

aaronj0 commented Sep 17, 2024

@vgvassilev, Should this change be extracted to a separate function GetStaticDatamembers? If we extract it to a separate function we will have to modify CPyCppyy slightly.

I think it is better to make this a separate interface, but we don't need changes in CPyCppyy. Since clingwrapper currently expects static data members with this interface we can stack the results ofCpp::GetDatamembers and then Cpp::GetStaticDatamembers, onto the same std::vector in the Cppyy::GetDatamembers interface

Copy link
Contributor

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

Copy link

codecov bot commented Sep 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.71%. Comparing base (33546ba) to head (4ff92da).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #322      +/-   ##
==========================================
+ Coverage   73.69%   73.71%   +0.01%     
==========================================
  Files           8        8              
  Lines        3019     3021       +2     
==========================================
+ Hits         2225     2227       +2     
  Misses        794      794              
Files with missing lines Coverage Δ
include/clang/Interpreter/CppInterOp.h 96.00% <ø> (ø)
lib/Interpreter/CppInterOp.cpp 79.57% <100.00%> (+0.02%) ⬆️
Files with missing lines Coverage Δ
include/clang/Interpreter/CppInterOp.h 96.00% <ø> (ø)
lib/Interpreter/CppInterOp.cpp 79.57% <100.00%> (+0.02%) ⬆️

@Vipul-Cariappa
Copy link
Collaborator Author

The failing tests need to be fixed in cppyy with the following patch:

diff --git a/python/cppyy/__init__.py b/python/cppyy/__init__.py
index 53b7e92..c5d7a3b 100644
--- a/python/cppyy/__init__.py
+++ b/python/cppyy/__init__.py
@@ -133,6 +133,7 @@ def _standard_pythonizations(pyclass, name):
                 return other == -1 or  int(self) == other
             def __ne__(self, other):
                 return other != -1 and int(self) != other
+        del pyclass.__class__.npos          # drop b/c is const data
         pyclass.npos = NPOS(pyclass.npos)
 
     return True

Taken from master.

Copy link
Contributor

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

return datamembers;
}

return {};
}

std::vector<TCppScope_t> GetStaticDatamembers(TCppScope_t scope) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should pass the std::vector as an out parameter.

Copy link
Contributor

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

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! @aaronj0, what do you think?

@aaronj0 aaronj0 merged commit 12e6e69 into compiler-research:main Sep 19, 2024
59 checks passed
@Vipul-Cariappa Vipul-Cariappa deleted the static-fields branch September 28, 2024 04:16
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants