Add full member function support in BestOverloadFunctionMatch#802
Add full member function support in BestOverloadFunctionMatch#802Priyanshu3820 wants to merge 15 commits intocompiler-research:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the BestOverloadFunctionMatch function to properly support const-qualified member function overload resolution, addressing issue #590. The implementation switches from using Clang's AddOverloadCandidate API to AddMethodCandidate and AddMethodTemplateCandidate APIs when resolving member function overloads with an invoking object type specified.
Changes:
- Added optional
invoking_object_typeparameter toBestOverloadFunctionMatchto enable overload resolution based on const-qualification and value category of the invoking object - Modified overload candidate selection logic to use appropriate Clang APIs (
AddMethodCandidatefor member functions,AddOverloadCandidatefor regular functions) - Added comprehensive test coverage for const-qualified member function overload resolution
- Applied code formatting improvements across test files
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| include/CppInterOp/CppInterOp.h | Added optional invoking_object_type parameter with default value to maintain backward compatibility |
| lib/CppInterOp/CppInterOp.cpp | Implemented logic to handle member function overload resolution with object type consideration; includes synthetic expression creation for the invoking object and refactored error message formatting |
| unittests/CppInterOp/FunctionReflectionTest.cpp | Added new test case for const-qualified member function overload resolution and applied code formatting improvements throughout |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Wasn't sure whether to check the new feature option but did it anyways. Also, a lot of diffs happened after running clang-format |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| WrapperExpr() : OpaqueValueExpr(clang::Stmt::EmptyShell()) {} | ||
| }; | ||
| auto* Exprs = new WrapperExpr[arg_types.size()]; | ||
| // Check if we need to prepend the invoking object (for member functions) |
There was a problem hiding this comment.
warning: initializing non-owner 'WrapperExpr *' with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory]
auto* Exprs = new WrapperExpr[num_exprs];
^| FunctionDecl* Result = Best != Overloads.end() ? Best->Function : nullptr; | ||
| FunctionDecl* Result = nullptr; | ||
|
|
||
| // If overload resolution succeeded or found an ambiguous match, use it |
There was a problem hiding this comment.
warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead [cppcoreguidelines-owning-memory]
delete[] Exprs;
^Additional context
lib/CppInterOp/CppInterOp.cpp:1296: variable declared here
auto* Exprs = new WrapperExpr[num_exprs];
^There was a problem hiding this comment.
warning: no header providing "size_t" is directly included [misc-include-cleaner]
unittests/CppInterOp/FunctionReflectionTest.cpp:9:
- #include <llvm/ADT/ArrayRef.h>
+ #include <cstddef>
+ #include <llvm/ADT/ArrayRef.h>|
|
||
| TYPED_TEST(CPPINTEROP_TEST_MODE, FunctionReflection_IsPublicMethod) { | ||
| std::vector<Decl *> Decls, SubDecls; | ||
| std::vector<Decl*> Decls, SubDecls; |
There was a problem hiding this comment.
warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
| std::vector<Decl*> Decls, SubDecls; | |
| std::vector<Decl*> Decls; | |
| std::vector<Decl*> SubDecls; |
There was a problem hiding this comment.
warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
| std::vector<Decl*> Decls, SubDecls; | |
| std::vector<Decl*> Decls; | |
| std::vector<Decl*> SubDecls; |
There was a problem hiding this comment.
warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
| std::vector<Decl*> Decls, SubDecls; | |
| std::vector<Decl*> Decls; | |
| std::vector<Decl*> SubDecls; |
There was a problem hiding this comment.
warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
| std::vector<Decl*> Decls, SubDecls; | |
| std::vector<Decl*> Decls; | |
| std::vector<Decl*> SubDecls; |
There was a problem hiding this comment.
warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
| std::vector<Decl*> Decls, SubDecls; | |
| std::vector<Decl*> Decls; | |
| std::vector<Decl*> SubDecls; |
There was a problem hiding this comment.
warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
| std::vector<Decl*> Decls, SubDecls; | |
| std::vector<Decl*> Decls; | |
| std::vector<Decl*> SubDecls; |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
(clang::CXXConstructorDecl*)Cpp::GetDefaultConstructor(Decls[0]);
^
| S.AddMethodTemplateCandidate( | ||
| FTD, DeclAccessPair::make(FTD, FTD->getAccess()), | ||
| cast<CXXRecordDecl>(FTD->getDeclContext()), &ExplicitTemplateArgs, | ||
| ObjectArg->getType(), ObjectArg->Classify(C), CallArgs, Overloads); |
There was a problem hiding this comment.
warning: isa_and_nonnull<> is preferred over an explicit test for null followed by calling isa<> [llvm-prefer-isa-or-dyn-cast-in-conditionals]
| ObjectArg->getType(), ObjectArg->Classify(C), CallArgs, Overloads); | |
| if (isa_and_nonnull<CXXMethodDecl>(FTD->getTemplatedDecl())) { |
There was a problem hiding this comment.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
void* args0[1] = {(void*)&i};
^There was a problem hiding this comment.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
void* args1[1] = {(void*)&s};
^There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
auto* CtorD = (clang::CXXConstructorDecl*)Cpp::GetDefaultConstructor(ClassC);
^There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
auto* DtorD = (clang::CXXDestructorDecl*)Cpp::GetDestructor(ClassC);
^There was a problem hiding this comment.
warning: no header providing "clang::CXXDestructorDecl" is directly included [misc-include-cleaner]
unittests/CppInterOp/FunctionReflectionTest.cpp:9:
- #include <llvm/ADT/ArrayRef.h>
+ #include <clang/AST/DeclCXX.h>
+ #include <llvm/ADT/ArrayRef.h>| operators DFLT_OP_ARITY); | ||
| EXPECT_EQ(operators.size(), 2); | ||
|
|
||
| auto kp_int_type = Cpp::GetTypeFromScope(KlassProduct_int); |
There was a problem hiding this comment.
warning: 'auto kp_int_type' can be declared as 'auto *kp_int_type' [llvm-qualified-auto]
| auto kp_int_type = Cpp::GetTypeFromScope(KlassProduct_int); | |
| auto *kp_int_type = Cpp::GetTypeFromScope(KlassProduct_int); |
| EXPECT_EQ(operators.size(), 2); | ||
|
|
||
| auto kp_int_type = Cpp::GetTypeFromScope(KlassProduct_int); | ||
| auto kp_int_lvalue = Cpp::GetReferencedType(kp_int_type, false); |
There was a problem hiding this comment.
warning: 'auto kp_int_lvalue' can be declared as 'auto *kp_int_lvalue' [llvm-qualified-auto]
| auto kp_int_lvalue = Cpp::GetReferencedType(kp_int_type, false); | |
| auto *kp_int_lvalue = Cpp::GetReferencedType(kp_int_type, false); |
| EXPECT_TRUE(where == Cpp::Construct(scope, where DFLT_1)); | ||
| // Check for the value of x which should be at the start of the object. | ||
| EXPECT_TRUE(*(int *)where == 12345); | ||
| EXPECT_TRUE(*(int*)where == 12345); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
EXPECT_TRUE(*(int*)where == 12345);
^There was a problem hiding this comment.
warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
auto* new_head = reinterpret_cast<void*>(reinterpret_cast<char*>(where) +
^
There was a problem hiding this comment.
Random formatting change. Revert.
There was a problem hiding this comment.
warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
auto* new_head = reinterpret_cast<void*>(reinterpret_cast<char*>(where) +
^There was a problem hiding this comment.
warning: unused variable 'C' [clang-diagnostic-unused-variable]
ASTContext& C = Interp->getCI()->getASTContext();
^There was a problem hiding this comment.
warning: no header providing "std::vector" is directly included [misc-include-cleaner]
unittests/CppInterOp/InterpreterTest.cpp:2:
+ #include <vector>There was a problem hiding this comment.
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]
auto* CXI = clang_createInterpreter(argv, 1);
^There was a problem hiding this comment.
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]
auto* CXI = clang_createInterpreter(argv, 3);
^There was a problem hiding this comment.
warning: no header providing "std::end" is directly included [misc-include-cleaner]
unittests/CppInterOp/InterpreterTest.cpp:2:
+ #include <iterator>
|
@mcbarton, actually these changes occured after running clang-format |
@Priyanshu3820 clang-format on the code may suggest these changes, but we use git clang format to only apply clang-format to the changes in the PR/commits. |
I get it now, I was supposed to run git-clang-format not clang-format on the files. I'll revert the changes(there are too many though) |
|
@mcbarton, I've fixed the formatting |
|
hi @mcbarton, can you please re-run the workflow once more, I am sure the tests would pass this time. All the tests pass locally. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #802 +/- ##
==========================================
+ Coverage 79.46% 79.56% +0.10%
==========================================
Files 11 11
Lines 4002 4056 +54
==========================================
+ Hits 3180 3227 +47
- Misses 822 829 +7
🚀 New features to boost your workflow:
|
This reverts commit 71b0529.
| bool has_invoking_object = (invoking_object_type != nullptr); | ||
| if (has_invoking_object) | ||
| num_exprs++; | ||
| auto* Exprs = new WrapperExpr[num_exprs]; |
There was a problem hiding this comment.
warning: initializing non-owner 'WrapperExpr *' with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory]
auto* Exprs = new WrapperExpr[num_exprs];
^| // If the templated declaration is a non-static method and we do not | ||
| // have an invoking object, skip adding it as a non-member candidate. | ||
| if (FTD->getTemplatedDecl() && | ||
| isa<CXXMethodDecl>(FTD->getTemplatedDecl())) { |
There was a problem hiding this comment.
warning: isa_and_nonnull<> is preferred over an explicit test for null followed by calling isa<> [llvm-prefer-isa-or-dyn-cast-in-conditionals]
| isa<CXXMethodDecl>(FTD->getTemplatedDecl())) { | |
| if (isa_and_nonnull<CXXMethodDecl>(FTD->getTemplatedDecl())) { |
| Result = Best->Function; | ||
| } | ||
|
|
||
| delete[] Exprs; |
There was a problem hiding this comment.
warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead [cppcoreguidelines-owning-memory]
delete[] Exprs;
^Additional context
lib/CppInterOp/CppInterOp.cpp:1296: variable declared here
auto* Exprs = new WrapperExpr[num_exprs];
^
|
@mcbarton, can you guide me how do we go forward in cases like this. Should I create a PR in cppyy-backend at the same time for simultaneous review or should I wait for this to land first? |
|
@Priyanshu3820 Although we have this situation come up occasionally I don't think we have a set procedure on how to deal with it. My suggestion is as follows.
|
2 participants
Description
This PR enhances
BestOverloadFunctionMatchto properly handle const-qualified member function overload resolution by utilizing Clang'sAddMethodCandidateAPI instead ofAddOverloadCandidate.Fixes #590
Type of change
Please tick all options which are relevant.
Testing
Tested calling with both const object type and non-const object type
Checklist