-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add a libclang-style C API and improve type safety #304
Conversation
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 233. Check the log or trigger a new build to see more.
How to tell clang-tidy to ignore the |
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 221. Check the log or trigger a new build to see more.
I feel like this PR is also addressing compiler-research/CPyCppyy#8 to some extent. Maybe we can use the momentum gained here.. |
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 27. Check the log or trigger a new build to see more.
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 18. Check the log or trigger a new build to see more.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #304 +/- ##
==========================================
- Coverage 73.71% 68.25% -5.47%
==========================================
Files 8 9 +1
Lines 3021 3830 +809
==========================================
+ Hits 2227 2614 +387
- Misses 794 1216 +422
... and 2 files with indirect coverage changes
|
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 12. Check the log or trigger a new build to see more.
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.
clang-tidy made some suggestions
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.
clang-tidy made some suggestions
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.
clang-tidy made some suggestions
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 13. Check the log or trigger a new build to see more.
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 24. Check the log or trigger a new build to see more.
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 58. Check the log or trigger a new build to see more.
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.
clang-tidy made some suggestions
lib/Interpreter/CXCppInterOp.cpp
Outdated
auto* I = getInterpreter(scope); | ||
if (const Cpp::JitCall JC = Cpp::MakeFunctionCallableImpl(I, getDecl(Ctor))) { | ||
if (arena) { | ||
JC.Invoke(arena, {}, (void*)~0); // Tell Invoke to use placement new. |
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.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
JC.Invoke(arena, {}, (void*)~0); // Tell Invoke to use placement new.
^
lib/Interpreter/CXCppInterOp.cpp
Outdated
auto* I = getInterpreter(scope); | ||
if (const Cpp::JitCall JC = Cpp::MakeFunctionCallableImpl(I, getDecl(Ctor))) { | ||
if (arena) { | ||
JC.Invoke(arena, {}, (void*)~0); // Tell Invoke to use placement new. |
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.
warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
JC.Invoke(arena, {}, (void*)~0); // Tell Invoke to use placement new.
^
auto *D = (Decl *) klass; | ||
std::string GetCompleteName(TCppScope_t klass) { | ||
auto& C = getSema().getASTContext(); | ||
auto* D = (Decl*)klass; |
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.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
auto* D = (Decl*)klass;
^
auto *D = (Decl *) klass; | ||
std::string GetQualifiedCompleteName(TCppScope_t klass) { | ||
auto& C = getSema().getASTContext(); | ||
auto* D = (Decl*)klass; |
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.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
auto* D = (Decl*)klass;
^
lib/Interpreter/CppInterOp.cpp
Outdated
if (!builtin.isNull()) | ||
return builtin; | ||
|
||
auto* D = (Decl*)GetNamedImpl(&sema, name, /* Within= */ 0); |
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.
warning: argument name 'Within' in comment does not match parameter name 'parent' [bugprone-argument-comment]
auto* D = (Decl*)GetNamedImpl(&sema, name, /* Within= */ 0);
^
Additional context
lib/Interpreter/CppInterOp.cpp:569: 'parent' declared here
Decl* parent /*= nullptr*/) {
^
lib/Interpreter/CppInterOp.cpp
Outdated
if (!builtin.isNull()) | ||
return builtin; | ||
|
||
auto* D = (Decl*)GetNamedImpl(&sema, name, /* Within= */ 0); |
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.
warning: use nullptr [modernize-use-nullptr]
auto* D = (Decl*)GetNamedImpl(&sema, name, /* Within= */ 0); | |
auto* D = (Decl*)GetNamedImpl(&sema, name, /* Within= */ nullptr); |
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.
clang-tidy made some suggestions
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.
This starts looking good. I wonder what's the way to test the C layer? I worry that it can regress without tests...
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.
clang-tidy made some suggestions
Hey, @Gnimuc, I was wondering what's the fate of this PR? Looks like we are almost there. |
I will revisit this and rebase it on the latest master next month. (I'm still testing the library in https://github.com/Gnimuc/CppCall.jl) |
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.
clang-tidy made some suggestions
auto* I = getInterpreter(scope); | ||
if (const Cpp::JitCall JC = Cpp::MakeFunctionCallableImpl(I, getDecl(Ctor))) { | ||
if (arena) { | ||
JC.Invoke(&arena, {}, (void*)~0); // Tell Invoke to use placement new. |
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.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
JC.Invoke(&arena, {}, (void*)~0); // Tell Invoke to use placement new.
^
auto* I = getInterpreter(scope); | ||
if (const Cpp::JitCall JC = Cpp::MakeFunctionCallableImpl(I, getDecl(Ctor))) { | ||
if (arena) { | ||
JC.Invoke(&arena, {}, (void*)~0); // Tell Invoke to use placement new. |
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.
warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
JC.Invoke(&arena, {}, (void*)~0); // Tell Invoke to use placement new.
^
D = GetUnderlyingScope(D); | ||
Within = llvm::dyn_cast<clang::DeclContext>(D); | ||
} | ||
|
||
auto *ND = Cpp_utils::Lookup::Named(&getSema(), name, Within); | ||
auto* ND = Cpp_utils::Lookup::Named(&getSema(), name, Within); |
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.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
auto* D = (clang::Decl*)parent;
^
{ | ||
if (!var) | ||
intptr_t GetVariableOffsetImpl(compat::Interpreter& I, Decl* D) { | ||
if (!D) |
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.
warning: 'auto DD' can be declared as 'auto *DD' [llvm-qualified-auto]
if (!D) | |
if (auto *DD = llvm::dyn_cast_or_null<DeclaratorDecl>(D)) { |
@@ -1227,6 +1220,11 @@ | |||
return 0; | |||
} | |||
|
|||
intptr_t GetVariableOffset(TCppScope_t var) { | |||
auto* D = static_cast<Decl*>(var); | |||
return GetVariableOffsetImpl(getInterp(), D); |
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.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
return (intptr_t) jitTargetAddressToPointer<void*>(VDAorErr.get());
^
if (const auto* Dtor = dyn_cast<CXXDestructorDecl>(D)) { | ||
if (auto Wrapper = make_dtor_wrapper(*interp, Dtor->getParent())) | ||
return {JitCall::kDestructorCall, Wrapper, Dtor}; | ||
// FIXME: else error we failed to compile the wrapper. |
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.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
return (JitCall::DestructorCall)F;
^
(*wrapper)(This, /*nary=*/0, withFree); | ||
return; | ||
} | ||
// FIXME: Diagnose. | ||
} | ||
|
||
void Destruct(TCppObject_t This, TCppScope_t scope, bool withFree /*=true*/) { | ||
auto* Class = static_cast<Decl*>(scope); | ||
DestructImpl(getInterp(), This, Class, withFree); |
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.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
JC.Invoke(&arena, {}, (void*)~0); // Tell Invoke to use placement new.
^
(*wrapper)(This, /*nary=*/0, withFree); | ||
return; | ||
} | ||
// FIXME: Diagnose. | ||
} | ||
|
||
void Destruct(TCppObject_t This, TCppScope_t scope, bool withFree /*=true*/) { | ||
auto* Class = static_cast<Decl*>(scope); | ||
DestructImpl(getInterp(), This, Class, withFree); |
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.
warning: integer to pointer cast pessimizes optimization opportunities [performance-no-int-to-ptr]
JC.Invoke(&arena, {}, (void*)~0); // Tell Invoke to use placement new.
^
I’m closing this because I feel that, instead of wrapping everything into one large PR, it’s better to proceed gradually. see #337 |
TODO:
void AddSearchPath(const char *dir, bool isUser, bool prepend);
const char* GetResourceDir();
void AddIncludePath(const char *dir);
int Declare(const char* code, bool silent);
int Process(const char *code);
intptr_t Evaluate(const char *code, bool *HadError/*=nullptr*/);
std::string LookupLibrary(const char* lib_name);
bool LoadLibrary(const char* lib_stem, bool lookup);
void UnloadLibrary(const char* lib_stem);
std::string SearchLibrariesForSymbol(const char* mangled_name, bool search_system /*true*/);
std::string ObjToString(const char *type, void *obj);
void Destruct(TCppObject_t This, TCppScope_t scope, bool withFree /*=true*/);
TCppFuncAddr_t GetFunctionAddress(const char* mangled_name);
CPPINTEROP_API JitCall MakeFunctionCallable(TCppConstFunction_t func);
intptr_t GetVariableOffset(TCppScope_t var);
bool InsertOrReplaceJitSymbol(const char* linker_mangled_name, uint64_t address);
Update:
Keeping the C++ API untouched is impossible if we want libclang-styled types. As a result, I reimplemented most of the C++ APIs on the C API side.