Skip to content
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

fix MakeFunctionCallable for dealing with public typedef to private type #407

Merged

Conversation

Vipul-Cariappa
Copy link
Collaborator

Description

Fixes MakeFunctionCallable's make_wrapper's code generation for the situation where a public typedef is used to point to a private type.

Fixes # (issue)

test24_typedef_to_private_class at cppyy.

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Requires documentation updates

Testing

Added in a test case replicating the cppyy's test.

Checklist

  • I have read the contribution guide recently

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

@@ -1652,13 +1655,13 @@ namespace Cpp {
return;
} else if (QT->isPointerType()) {
isPointer = true;
QT = cast<clang::PointerType>(QT)->getPointeeType();
QT = cast<clang::PointerType>(QT.getCanonicalType())->getPointeeType();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "clang::cast" is directly included [misc-include-cleaner]

        QT = cast<clang::PointerType>(QT.getCanonicalType())->getPointeeType();
             ^

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add test covering that line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. 👍🏽

EXPECT_TRUE(FCI_f.getKind() == Cpp::JitCall::kGenericCall);

void* res = nullptr;
FCI_f.Invoke(&res, {nullptr, 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: multilevel pointer conversion from 'void **' to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

  FCI_f.Invoke(&res, {nullptr, 0});
               ^

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.96%. Comparing base (c446c3e) to head (d1ffcd8).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #407      +/-   ##
==========================================
+ Coverage   70.85%   70.96%   +0.10%     
==========================================
  Files           9        9              
  Lines        3538     3541       +3     
==========================================
+ Hits         2507     2513       +6     
+ Misses       1031     1028       -3     
Files with missing lines Coverage Δ
lib/Interpreter/CppInterOp.cpp 80.34% <100.00%> (+0.19%) ⬆️
Files with missing lines Coverage Δ
lib/Interpreter/CppInterOp.cpp 80.34% <100.00%> (+0.19%) ⬆️

@Vipul-Cariappa Vipul-Cariappa force-pushed the dev/private-typedef branch 2 times, most recently from 2257e1e to 667f38e Compare January 4, 2025 04:37
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

EXPECT_EQ(set_5_f.getKind(), Cpp::JitCall::kGenericCall);

int* bp = &b;
void* set_5_args[1] = {(void*)&bp};
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 declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]

  void* set_5_args[1] = {(void*)&bp};
  ^

EXPECT_EQ(set_5_f.getKind(), Cpp::JitCall::kGenericCall);

int* bp = &b;
void* set_5_args[1] = {(void*)&bp};
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: multilevel pointer conversion from 'int **' to 'void *', please use explicit cast [bugprone-multi-level-implicit-pointer-conversion]

  void* set_5_args[1] = {(void*)&bp};
                                ^


int* bp = &b;
void* set_5_args[1] = {(void*)&bp};
set_5_f.Invoke(nullptr, {set_5_args, 1});
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 implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]

  set_5_f.Invoke(nullptr, {set_5_args, 1});
                           ^

Comment on lines +1605 to +1607
#if CLANG_VERSION_MAJOR > 16
Policy.SuppressElaboration = true;
#endif
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The SuppressElaboration Printing Policy is not part of clang <= 16, therefor CI fails.

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

set_5_f.Invoke(nullptr, {set_5_args, 1});
EXPECT_EQ(b, 5);

#if CLANG_VERSION_MAJOR > 16
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "CLANG_VERSION_MAJOR" is directly included [misc-include-cleaner]

unittests/CppInterOp/FunctionReflectionTest.cpp:9:

- #include <string>
+ #include <clang/Basic/Version.h>
+ #include <string>

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

#include "clang/Frontend/CompilerInstance.h"
#include "clang/Interpreter/CppInterOp.h"
#include "clang/Sema/Sema.h"

#include "gtest/gtest.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'gtest/gtest.h' file not found [clang-diagnostic-error]

#include "gtest/gtest.h"
         ^

@@ -4,5 +4,8 @@ Checks: >
-cppcoreguidelines-macro-usage,
-cppcoreguidelines-pro-type-cstyle-cast,
-cppcoreguidelines-avoid-non-const-global-variables,
-cppcoreguidelines-avoid-c-arrays,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we ignore running clang-tidy on the unittest folder instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have disabled these tests in the unittests folder.
Do you want to disable all the checks for the unittests folder?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably start disabling one by one. Maybe some checks still make sense...

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!

@Vipul-Cariappa Vipul-Cariappa merged commit 1bc6be2 into compiler-research:main Jan 10, 2025
47 checks passed
@Vipul-Cariappa Vipul-Cariappa deleted the dev/private-typedef branch January 10, 2025 03:37
Vipul-Cariappa added a commit to Vipul-Cariappa/cppyy-compiler-research that referenced this pull request Jan 10, 2025
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