-
Notifications
You must be signed in to change notification settings - Fork 14
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
Use reflect_invoke
metafunction to invoke a member function
#79
Use reflect_invoke
metafunction to invoke a member function
#79
Conversation
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
…and overloadd Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
a8e6784
to
16e63c7
Compare
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
I've added everything that I wanted, for now. |
libcxx/test/std/experimental/reflection/reflect-invoke.pass.cpp
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,65 @@ | |||
//===----------------------------------------------------------------------===// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea to add these!!
We desperately need more of these "fail-tests" for reflection (I haven't written any) - after you're finishing with this PR, if you have any interest in adding some, that'd be a huge help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind to work on it!
It would be helpful for me if you could elaborate what testing would be useful for authors of paper and users of this fork.
Should it be only different tests to trigger diagnostic messages or I also need to try to cover some branches where we just say that expression is invalid (return true;
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both would be great - we don't need full coverage of "expression is invalid" cases for every function, but a few would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, will do after this PR 👍🏻
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
…onstexpr function Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
@katzdm ready for the next iteration of review, when you have time |
clang/lib/Sema/Metafunctions.cpp
Outdated
if (auto *MD = dyn_cast<CXXMethodDecl>(VD)) { | ||
// method declaration | ||
return MD; | ||
} else { | ||
// pointer to non-static method | ||
// validation was done in is_nonstatic_member_function | ||
auto *VarD = cast<VarDecl>(VD); | ||
if (!VarD->isConstexpr()) { | ||
// avoid extracting constexpr method declaration | ||
// from non-constexpr function pointers | ||
return nullptr; | ||
} | ||
|
||
Expr *Init = VarD->getInit(); | ||
|
||
if (auto *UnaryOp = dyn_cast<UnaryOperator>(Init)) { | ||
// get the operand of & | ||
Init = UnaryOp->getSubExpr(); | ||
} | ||
|
||
while (auto *CastExpr = dyn_cast<ImplicitCastExpr>(Init)) { | ||
// handle ImplicitCastExpr if it is wrapping the actual expression | ||
Init = CastExpr->getSubExpr(); | ||
} | ||
|
||
if (auto *DREFromUnary = dyn_cast<DeclRefExpr>(Init)) { | ||
return dyn_cast<CXXMethodDecl>(DREFromUnary->getDecl()); | ||
} | ||
} | ||
|
||
// if we reach this point | ||
// then probably there is a bug in `is_nonstatic_member_function` | ||
// or we failed to extract method from pointer | ||
llvm_unreachable("failed to get member function from decl ref expression"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that introspecting the substructure of the DeclRefExpr
is the best approach here. We could instead evaluate it as a prvalue, obtain an APValue
, and pull the CXXMethodDecl
out of there.
One of the kinds that an APValue
might be is MemberPointer
, for which it looks like you can use this method to get the ValueDecl
. Maybe we can just check if that ValueDecl
is a CXXMethodDecl
, and return that?
Feel free to ping me if more detail would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Let me know if it looks similar to what you expected
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Signed-off-by: Valentyn Yukhymenko <valentin.yukhymenko@gmail.com>
Issue number of the reported bug or feature request: #5
Describe your changes
Added support for calling non-static member functions via
reflect_invoke
if first provided argument is a reflection of a related object.Testing performed
Additional context
N/A