Skip to content

Refactoring the API for multiple interpreter instance support #338

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 1 commit into from
Oct 27, 2024

Conversation

Gnimuc
Copy link
Contributor

@Gnimuc Gnimuc commented Oct 27, 2024

I feel like we also need to split the implementation and use an internal shared header for 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

void Destruct(TCppObject_t This, TCppScope_t scope, bool withFree /*=true*/) {
Decl* Class = (Decl*)scope;
if (auto wrapper = make_dtor_wrapper(getInterp(), Class)) {
TCppObject_t Construct(TCppScope_t scope, void* arena /*=nullptr*/) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

also removed the Impl suffix.

Copy link

codecov bot commented Oct 27, 2024

Codecov Report

Attention: Patch coverage is 95.83333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.43%. Comparing base (c0a4dfe) to head (4ec28eb).

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

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #338      +/-   ##
==========================================
+ Coverage   74.29%   74.43%   +0.14%     
==========================================
  Files           8        8              
  Lines        3190     3204      +14     
==========================================
+ Hits         2370     2385      +15     
+ Misses        820      819       -1     
Files with missing lines Coverage Δ
include/clang/Interpreter/CppInterOp.h 96.29% <ø> (ø)
lib/Interpreter/CppInterOp.cpp 79.98% <95.83%> (+0.15%) ⬆️

... and 1 file with indirect coverage changes

Files with missing lines Coverage Δ
include/clang/Interpreter/CppInterOp.h 96.29% <ø> (ø)
lib/Interpreter/CppInterOp.cpp 79.98% <95.83%> (+0.15%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Contributor

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

@Gnimuc Gnimuc requested a review from vgvassilev October 27, 2024 13:28
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! Thank you, @Gnimuc!

@vgvassilev
Copy link
Contributor

Can you squash the commits to have clear history?

also fix argument type typos and c-style casts
@Gnimuc
Copy link
Contributor Author

Gnimuc commented Oct 27, 2024

Can you squash the commits to have clear history?

Done!

@vgvassilev vgvassilev merged commit 29588be into compiler-research:main Oct 27, 2024
# 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.

2 participants