-
Notifications
You must be signed in to change notification settings - Fork 39
Add CppInterOp API dispatch mechanism #730
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #730 +/- ##
==========================================
- Coverage 79.09% 79.02% -0.08%
==========================================
Files 9 11 +2
Lines 3898 3923 +25
==========================================
+ Hits 3083 3100 +17
- Misses 815 823 +8
🚀 New features to boost your workflow:
|
e259123 to
badab56
Compare
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 269. Check the log or trigger a new build to see more.
badab56 to
3f72c0b
Compare
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 259. Check the log or trigger a new build to see more.
|
|
||
| #include <CppInterOp/CppInterOp.h> | ||
|
|
||
| using __CPP_FUNC = void (*)(); |
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: declaration uses identifier '__CPP_FUNC', which is a reserved identifier [bugprone-reserved-identifier]
| using __CPP_FUNC = void (*)(); | |
| using CPP_FUNC = void (*)(); |
include/CppInterOp/CppInterOpDispatch.h:24:
- __CPP_FUNC address;
+ CPP_FUNC address;|
|
||
| #include <unordered_map> | ||
|
|
||
| static const FunctionEntry api_function_table[] = { |
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 declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
static const FunctionEntry api_function_table[] = {
^| #include <unordered_map> | ||
|
|
||
| static const FunctionEntry api_function_table[] = { | ||
| {"GetInterpreter", (__CPP_FUNC)Cpp::GetInterpreter}, |
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]
{"GetInterpreter", (__CPP_FUNC)Cpp::GetInterpreter},
^| #include <unordered_map> | ||
|
|
||
| static const FunctionEntry api_function_table[] = { | ||
| {"GetInterpreter", (__CPP_FUNC)Cpp::GetInterpreter}, |
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: no header providing "Cpp::GetInterpreter" is directly included [misc-include-cleaner]
lib/CppInterOp/CppInterOpDispatch.cpp:10:
- #include <CppInterOp/CppInterOpDispatch.h>
+ #include <CppInterOp/CppInterOp.h>
+ #include <CppInterOp/CppInterOpDispatch.h>|
|
||
| static const FunctionEntry api_function_table[] = { | ||
| {"GetInterpreter", (__CPP_FUNC)Cpp::GetInterpreter}, | ||
| {"CreateInterpreter", (__CPP_FUNC)Cpp::CreateInterpreter}, |
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]
{"CreateInterpreter", (__CPP_FUNC)Cpp::CreateInterpreter},
^|
|
||
| static const FunctionEntry api_function_table[] = { | ||
| {"GetInterpreter", (__CPP_FUNC)Cpp::GetInterpreter}, | ||
| {"CreateInterpreter", (__CPP_FUNC)Cpp::CreateInterpreter}, |
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: no header providing "Cpp::CreateInterpreter" is directly included [misc-include-cleaner]
{"CreateInterpreter", (__CPP_FUNC)Cpp::CreateInterpreter},
^| static const FunctionEntry api_function_table[] = { | ||
| {"GetInterpreter", (__CPP_FUNC)Cpp::GetInterpreter}, | ||
| {"CreateInterpreter", (__CPP_FUNC)Cpp::CreateInterpreter}, | ||
| {"Process", (__CPP_FUNC)Cpp::Process}, |
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]
{"Process", (__CPP_FUNC)Cpp::Process},
^| static const FunctionEntry api_function_table[] = { | ||
| {"GetInterpreter", (__CPP_FUNC)Cpp::GetInterpreter}, | ||
| {"CreateInterpreter", (__CPP_FUNC)Cpp::CreateInterpreter}, | ||
| {"Process", (__CPP_FUNC)Cpp::Process}, |
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: no header providing "Cpp::Process" is directly included [misc-include-cleaner]
{"Process", (__CPP_FUNC)Cpp::Process},
^| {"GetInterpreter", (__CPP_FUNC)Cpp::GetInterpreter}, | ||
| {"CreateInterpreter", (__CPP_FUNC)Cpp::CreateInterpreter}, | ||
| {"Process", (__CPP_FUNC)Cpp::Process}, | ||
| {"GetResourceDir", (__CPP_FUNC)Cpp::GetResourceDir}, |
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]
{"GetResourceDir", (__CPP_FUNC)Cpp::GetResourceDir},
^| {"GetInterpreter", (__CPP_FUNC)Cpp::GetInterpreter}, | ||
| {"CreateInterpreter", (__CPP_FUNC)Cpp::CreateInterpreter}, | ||
| {"Process", (__CPP_FUNC)Cpp::Process}, | ||
| {"GetResourceDir", (__CPP_FUNC)Cpp::GetResourceDir}, |
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: no header providing "Cpp::GetResourceDir" is directly included [misc-include-cleaner]
{"GetResourceDir", (__CPP_FUNC)Cpp::GetResourceDir},
^| // a consistent way. This is used as our dispatched API list, along with the | ||
| // name-address pair table | ||
| #define FOR_EACH_CPP_FUNCTION(DO) \ | ||
| DO(CreateInterpreter) \ |
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 can take a pair <CreateInterpreter, Type> and in this case you can write the using clauses automatically. We can also use clang-tablegen to generate these for every function in the namespace Cpp.
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.
Ping on this. We want to be writing the using clauses automatically.
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.
Looks like this is indeed possible.
#define EXTERN_CPP_FUNC_SIMPLE(func_name) \
extern CppAPIType::func_name func_name;
#define EXTERN_CPP_FUNC_OVERLOADED(func_name, signature) \
extern CppAPIType::func_name func_name;
#define LOAD_CPP_FUNCTION_SIMPLE(func_name) \
func_name = reinterpret_cast<CppAPIType::func_name>(dlGetProcAddress(#func_name));
#define LOAD_CPP_FUNCTION_OVERLOADED(func_name, signature) \
func_name = reinterpret_cast<CppAPIType::func_name>(dlGetProcAddress(#func_name));
#define DECLARE_CPP_NULL_SIMPLE(func_name) \
CppAPIType::func_name func_name = nullptr;
#define DECLARE_CPP_NULL_OVERLOADED(func_name, signature) \
CppAPIType::func_name func_name = nullptr;
// macro that allows declaration and loading of all CppInterOp API functions in
// a consistent way. This is used as our dispatched API list, along with the
// name-address pair table
#define FOR_EACH_CPP_FUNCTION_SIMPLE(DO) \
DO(CreateInterpreter) \
DO(GetInterpreter) \
// ... rest of non overloaded API
DO(EnableDebugOutput)
#define FOR_EACH_CPP_FUNCTION_OVERLOADED(DO) \
DO(MakeFunctionCallable, Cpp::JitCall (*)(Cpp::TCppConstFunction_t)) \
DO(GetFunctionAddress, Cpp::TCppFuncAddr_t (*)(Cpp::TCppFunction_t))
#define EXTRACT_NAME_OVERLOADED(name, sig) name
#define FOR_EACH_CPP_FUNCTION(DO) \
FOR_EACH_CPP_FUNCTION_SIMPLE(DO) \
FOR_EACH_CPP_FUNCTION_OVERLOADED(EXTRACT_NAME_OVERLOADED)
#define DECLARE_TYPE_SIMPLE(func_name) \
using func_name = decltype(&Cpp::func_name);
#define DECLARE_TYPE_OVERLOADED(func_name, signature) \
using func_name = signature;
namespace CppDispatch {
// Forward all type aliases
using TCppIndex_t = ::Cpp::TCppIndex_t;
...
using JitCall = ::Cpp::JitCall;
} // end namespace CppDispatch
namespace CppAPIType {
FOR_EACH_CPP_FUNCTION_SIMPLE(DECLARE_TYPE_SIMPLE)
FOR_EACH_CPP_FUNCTION_OVERLOADED(DECLARE_TYPE_OVERLOADED)
} // end namespace CppAPIType
#undef DECLARE_TYPE_SIMPLE
#undef DECLARE_TYPE_OVERLOADEDThere 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.
Here FOR_EACH_CPP_FUNCTION_SIMPLE and FOR_EACH_CPP_FUNCTION_OVERLOADED serve as the only ground truth for where we keep the API list. If you have an overloaded API, you put it in the overloaded version as <API name, function pointer type>
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.
Of course all usages of the previous macro split up:
- FOR_EACH_CPP_FUNCTION(LOAD_CPP_FUNCTION)
+ FOR_EACH_CPP_FUNCTION_SIMPLE(LOAD_CPP_FUNCTION_SIMPLE)
+ FOR_EACH_CPP_FUNCTION_OVERLOADED(LOAD_CPP_FUNCTION_OVERLOADED)
- FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC)
+ FOR_EACH_CPP_FUNCTION_SIMPLE(EXTERN_CPP_FUNC_SIMPLE)
+ FOR_EACH_CPP_FUNCTION_OVERLOADED(EXTERN_CPP_FUNC_OVERLOADED)But that keeps the whole thing automatic
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.
Probably you don't need such a complexity but a X-macro that expands. You can also take the type of the function using decltype and probably handle the overloads there, too. It seems that you already use decltype but I do not understand why this can be a simpler macro setup..
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.
Probably you don't need such a complexity but a X-macro that expands.
Can you give me a clearer idea on that?
You can also take the type of the function using decltype and probably handle the overloads there, too.
Well, yes I can make this simpler if I write the signatures for all API and take the decltype of the function obtained on static_cast. I've split up the macros to avoid static_cast on non-overloaded methods, as that seems like an unnecessary operation...
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.
3f72c0b to
624d59a
Compare
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 249. Check the log or trigger a new build to see more.
| {"CreateInterpreter", (__CPP_FUNC)Cpp::CreateInterpreter}, | ||
| {"Process", (__CPP_FUNC)Cpp::Process}, | ||
| {"GetResourceDir", (__CPP_FUNC)Cpp::GetResourceDir}, | ||
| {"AddIncludePath", (__CPP_FUNC)Cpp::AddIncludePath}, |
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]
{"AddIncludePath", (__CPP_FUNC)Cpp::AddIncludePath},
^| {"CreateInterpreter", (__CPP_FUNC)Cpp::CreateInterpreter}, | ||
| {"Process", (__CPP_FUNC)Cpp::Process}, | ||
| {"GetResourceDir", (__CPP_FUNC)Cpp::GetResourceDir}, | ||
| {"AddIncludePath", (__CPP_FUNC)Cpp::AddIncludePath}, |
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: no header providing "Cpp::AddIncludePath" is directly included [misc-include-cleaner]
{"AddIncludePath", (__CPP_FUNC)Cpp::AddIncludePath},
^| {"Process", (__CPP_FUNC)Cpp::Process}, | ||
| {"GetResourceDir", (__CPP_FUNC)Cpp::GetResourceDir}, | ||
| {"AddIncludePath", (__CPP_FUNC)Cpp::AddIncludePath}, | ||
| {"LoadLibrary", (__CPP_FUNC)Cpp::LoadLibrary}, |
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]
{"LoadLibrary", (__CPP_FUNC)Cpp::LoadLibrary},
^| {"Process", (__CPP_FUNC)Cpp::Process}, | ||
| {"GetResourceDir", (__CPP_FUNC)Cpp::GetResourceDir}, | ||
| {"AddIncludePath", (__CPP_FUNC)Cpp::AddIncludePath}, | ||
| {"LoadLibrary", (__CPP_FUNC)Cpp::LoadLibrary}, |
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: no header providing "Cpp::LoadLibrary" is directly included [misc-include-cleaner]
{"LoadLibrary", (__CPP_FUNC)Cpp::LoadLibrary},
^| {"GetResourceDir", (__CPP_FUNC)Cpp::GetResourceDir}, | ||
| {"AddIncludePath", (__CPP_FUNC)Cpp::AddIncludePath}, | ||
| {"LoadLibrary", (__CPP_FUNC)Cpp::LoadLibrary}, | ||
| {"Declare", (__CPP_FUNC)Cpp::Declare}, |
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]
{"Declare", (__CPP_FUNC)Cpp::Declare},
^| {"GetResourceDir", (__CPP_FUNC)Cpp::GetResourceDir}, | ||
| {"AddIncludePath", (__CPP_FUNC)Cpp::AddIncludePath}, | ||
| {"LoadLibrary", (__CPP_FUNC)Cpp::LoadLibrary}, | ||
| {"Declare", (__CPP_FUNC)Cpp::Declare}, |
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: no header providing "Cpp::Declare" is directly included [misc-include-cleaner]
{"Declare", (__CPP_FUNC)Cpp::Declare},
^| {"AddIncludePath", (__CPP_FUNC)Cpp::AddIncludePath}, | ||
| {"LoadLibrary", (__CPP_FUNC)Cpp::LoadLibrary}, | ||
| {"Declare", (__CPP_FUNC)Cpp::Declare}, | ||
| {"DeleteInterpreter", (__CPP_FUNC)Cpp::DeleteInterpreter}, |
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]
{"DeleteInterpreter", (__CPP_FUNC)Cpp::DeleteInterpreter},
^| {"AddIncludePath", (__CPP_FUNC)Cpp::AddIncludePath}, | ||
| {"LoadLibrary", (__CPP_FUNC)Cpp::LoadLibrary}, | ||
| {"Declare", (__CPP_FUNC)Cpp::Declare}, | ||
| {"DeleteInterpreter", (__CPP_FUNC)Cpp::DeleteInterpreter}, |
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: no header providing "Cpp::DeleteInterpreter" is directly included [misc-include-cleaner]
{"DeleteInterpreter", (__CPP_FUNC)Cpp::DeleteInterpreter},
^| {"LoadLibrary", (__CPP_FUNC)Cpp::LoadLibrary}, | ||
| {"Declare", (__CPP_FUNC)Cpp::Declare}, | ||
| {"DeleteInterpreter", (__CPP_FUNC)Cpp::DeleteInterpreter}, | ||
| {"IsNamespace", (__CPP_FUNC)Cpp::IsNamespace}, |
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]
{"IsNamespace", (__CPP_FUNC)Cpp::IsNamespace},
^| {"LoadLibrary", (__CPP_FUNC)Cpp::LoadLibrary}, | ||
| {"Declare", (__CPP_FUNC)Cpp::Declare}, | ||
| {"DeleteInterpreter", (__CPP_FUNC)Cpp::DeleteInterpreter}, | ||
| {"IsNamespace", (__CPP_FUNC)Cpp::IsNamespace}, |
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: no header providing "Cpp::IsNamespace" is directly included [misc-include-cleaner]
{"IsNamespace", (__CPP_FUNC)Cpp::IsNamespace},
^624d59a to
460f493
Compare
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 126. Check the log or trigger a new build to see more.
| {"Declare", (__CPP_FUNC)Cpp::Declare}, | ||
| {"DeleteInterpreter", (__CPP_FUNC)Cpp::DeleteInterpreter}, | ||
| {"IsNamespace", (__CPP_FUNC)Cpp::IsNamespace}, | ||
| {"ObjToString", (__CPP_FUNC)Cpp::ObjToString}, |
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]
{"ObjToString", (__CPP_FUNC)Cpp::ObjToString},
^| {"DeleteInterpreter", (__CPP_FUNC)Cpp::DeleteInterpreter}, | ||
| {"IsNamespace", (__CPP_FUNC)Cpp::IsNamespace}, | ||
| {"ObjToString", (__CPP_FUNC)Cpp::ObjToString}, | ||
| {"GetQualifiedCompleteName", (__CPP_FUNC)Cpp::GetQualifiedCompleteName}, |
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]
{"GetQualifiedCompleteName", (__CPP_FUNC)Cpp::GetQualifiedCompleteName},
^| {"IsNamespace", (__CPP_FUNC)Cpp::IsNamespace}, | ||
| {"ObjToString", (__CPP_FUNC)Cpp::ObjToString}, | ||
| {"GetQualifiedCompleteName", (__CPP_FUNC)Cpp::GetQualifiedCompleteName}, | ||
| {"IsLValueReferenceType", (__CPP_FUNC)Cpp::IsLValueReferenceType}, |
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]
{"IsLValueReferenceType", (__CPP_FUNC)Cpp::IsLValueReferenceType},
^| {"ObjToString", (__CPP_FUNC)Cpp::ObjToString}, | ||
| {"GetQualifiedCompleteName", (__CPP_FUNC)Cpp::GetQualifiedCompleteName}, | ||
| {"IsLValueReferenceType", (__CPP_FUNC)Cpp::IsLValueReferenceType}, | ||
| {"GetNonReferenceType", (__CPP_FUNC)Cpp::GetNonReferenceType}, |
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]
{"GetNonReferenceType", (__CPP_FUNC)Cpp::GetNonReferenceType},
^| {"GetQualifiedCompleteName", (__CPP_FUNC)Cpp::GetQualifiedCompleteName}, | ||
| {"IsLValueReferenceType", (__CPP_FUNC)Cpp::IsLValueReferenceType}, | ||
| {"GetNonReferenceType", (__CPP_FUNC)Cpp::GetNonReferenceType}, | ||
| {"IsEnumType", (__CPP_FUNC)Cpp::IsEnumType}, |
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]
{"IsEnumType", (__CPP_FUNC)Cpp::IsEnumType},
^| {"IsLValueReferenceType", (__CPP_FUNC)Cpp::IsLValueReferenceType}, | ||
| {"GetNonReferenceType", (__CPP_FUNC)Cpp::GetNonReferenceType}, | ||
| {"IsEnumType", (__CPP_FUNC)Cpp::IsEnumType}, | ||
| {"GetIntegerTypeFromEnumType", (__CPP_FUNC)Cpp::GetIntegerTypeFromEnumType}, |
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]
{"GetIntegerTypeFromEnumType", (__CPP_FUNC)Cpp::GetIntegerTypeFromEnumType},
^| {"GetNonReferenceType", (__CPP_FUNC)Cpp::GetNonReferenceType}, | ||
| {"IsEnumType", (__CPP_FUNC)Cpp::IsEnumType}, | ||
| {"GetIntegerTypeFromEnumType", (__CPP_FUNC)Cpp::GetIntegerTypeFromEnumType}, | ||
| {"GetReferencedType", (__CPP_FUNC)Cpp::GetReferencedType}, |
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]
{"GetReferencedType", (__CPP_FUNC)Cpp::GetReferencedType},
^| {"IsEnumType", (__CPP_FUNC)Cpp::IsEnumType}, | ||
| {"GetIntegerTypeFromEnumType", (__CPP_FUNC)Cpp::GetIntegerTypeFromEnumType}, | ||
| {"GetReferencedType", (__CPP_FUNC)Cpp::GetReferencedType}, | ||
| {"IsPointerType", (__CPP_FUNC)Cpp::IsPointerType}, |
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]
{"IsPointerType", (__CPP_FUNC)Cpp::IsPointerType},
^| {"GetIntegerTypeFromEnumType", (__CPP_FUNC)Cpp::GetIntegerTypeFromEnumType}, | ||
| {"GetReferencedType", (__CPP_FUNC)Cpp::GetReferencedType}, | ||
| {"IsPointerType", (__CPP_FUNC)Cpp::IsPointerType}, | ||
| {"GetPointeeType", (__CPP_FUNC)Cpp::GetPointeeType}, |
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]
{"GetPointeeType", (__CPP_FUNC)Cpp::GetPointeeType},
^| {"GetReferencedType", (__CPP_FUNC)Cpp::GetReferencedType}, | ||
| {"IsPointerType", (__CPP_FUNC)Cpp::IsPointerType}, | ||
| {"GetPointeeType", (__CPP_FUNC)Cpp::GetPointeeType}, | ||
| {"GetPointerType", (__CPP_FUNC)Cpp::GetPointerType}, |
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]
{"GetPointerType", (__CPP_FUNC)Cpp::GetPointerType},
^| add_llvm_library(clangCppInterOp | ||
| DISABLE_LLVM_LINK_LLVM_DYLIB | ||
| CppInterOp.cpp | ||
| CppInterOpDispatch.cpp |
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.
Maybe we should name the infrastructure we built with something closer to what it does, eg. DyldDispatch.
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.
ping.
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.
#include <CppInterOp/DyldDispatch.h>?
This defines the mechanism which enables dispatching of the CppInterOp API without linking to it, preventing any LLVM or Clang symbols from being leaked into the client application. Can be used to deploy CppInterOp in an environment where dynamic linking is not favourable and the only option is dlopen'ing `libClangCppInterOp.so` with `RTLD_LOCAL`
to be followed up with a section in docs
3b9e2b4 to
795db00
Compare
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 243. Check the log or trigger a new build to see more.
| #define CPPINTEROP_LIBRARY_PATH "@CPPINTEROP_LIBRARY_PATH@" | ||
| #endif | ||
|
|
||
| using __CPP_FUNC = void (*)(); |
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: declaration uses identifier '__CPP_FUNC', which is a reserved identifier [bugprone-reserved-identifier]
| using __CPP_FUNC = void (*)(); | |
| using CPP_FUNC = void (*)(); |
| return; | ||
| } | ||
|
|
||
| getCppProcAddress = reinterpret_cast<void* (*)(const char*)>( |
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 reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
getCppProcAddress = reinterpret_cast<void* (*)(const char*)>(
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'AddIncludePath' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:57: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(AddIncludePath) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'Allocate' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:104: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(Allocate) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'BestOverloadFunctionMatch' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:141: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(BestOverloadFunctionMatch) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'Construct' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:106: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(Construct) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'CreateInterpreter' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:53: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(CreateInterpreter) \
^…part of the header
A client of this mechanis now only needs to do:
```
namespace CppDispatch {
// needed to keep the definitions in this translation unit
FOR_EACH_CPP_FUNCTION(DECLARE_CPP_NULL);
}
bool client::load_cpp() {
return CppDispatch::init_functions("/path/to/libclangCppInterOp.so");
}
```
795db00 to
a768abb
Compare
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 232. Check the log or trigger a new build to see more.
| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'Deallocate' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:105: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(Deallocate) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'Declare' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:59: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(Declare) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'DeleteInterpreter' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:60: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(DeleteInterpreter) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'Demangle' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:99: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(Demangle) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'Destruct' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:107: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(Destruct) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'DumpScope' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:165: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(DumpScope) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'EnableDebugOutput' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:169: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(EnableDebugOutput)
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'Evaluate' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:167: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(Evaluate) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'ExistsFunctionTemplate' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:137: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(ExistsFunctionTemplate) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'GetAllCppNames' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:116: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(GetAllCppNames) \
^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 218. Check the log or trigger a new build to see more.
| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'GetBaseClass' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:123: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(GetBaseClass) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'GetBaseClassOffset' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:127: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(GetBaseClassOffset) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'GetCanonicalType' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:74: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(GetCanonicalType) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'GetClassMethods' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:128: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(GetClassMethods) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'GetClassTemplateInstantiationArgs' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:83: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(GetClassTemplateInstantiationArgs) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'GetClassTemplatedMethods' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:140: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(GetClassTemplatedMethods) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'GetCompleteName' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:118: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(GetCompleteName) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'GetComplexType' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:87: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(GetComplexType) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'GetDatamembers' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:149: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(GetDatamembers) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC); |
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: variable 'GetDestructor' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION(EXTERN_CPP_FUNC);
^Additional context
include/CppInterOp/CppInterOpDispatch.h:119: expanded from macro 'FOR_EACH_CPP_FUNCTION'
DO(GetDestructor) \
^| #include <vector> | ||
|
|
||
| #include <cstdlib> | ||
| #include <dlfcn.h> |
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.
needs something like:
#ifdef _WIN32
#include <io.h>
#include <Psapi.h>
#define RTLD_DEFAULT ((void *)::GetModuleHandle(NULL))
//#define dlsym(library, function_name) ::GetProcAddress((HMODULE)library, function_name)
#define dlopen(library_name, flags) ::LoadLibrary(library_name)
#define dlclose(library) ::FreeLibrary((HMODULE)library)
#endif| // Configured by CMake, can be overridden by defining before including this | ||
| // header | ||
| #ifndef CPPINTEROP_LIBRARY_PATH | ||
| #define CPPINTEROP_LIBRARY_PATH "@CPPINTEROP_LIBRARY_PATH@" |
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.
That does not seem correct. The library can be installed or relocated.
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 is not the build location, but a new flag. The path can be overridden to the installed/relocated location either with CMake or by setting an env var. The header has no way of knowing that...
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.
That is still a problem, however, I do not understand why you need to define it if it is provided from the outside...
| {"IsFunctionPointerType", (__CPP_FUNC)Cpp::IsFunctionPointerType}, | ||
| {"GetVariableType", (__CPP_FUNC)Cpp::GetVariableType}, | ||
| {"GetNamed", (__CPP_FUNC)Cpp::GetNamed}, | ||
| {"GetScopeFromType", (__CPP_FUNC)Cpp::GetScopeFromType}, |
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.
These can be written by a macro, too.
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.
Mostly but we need to append the entries for overloaded APIs:
{"MakeFunctionCallable",
(__CPP_FUNC) static_cast<Cpp::JitCall (*)(Cpp::TCppConstFunction_t)>(
&Cpp::MakeFunctionCallable)},
{"GetFunctionAddress",
(__CPP_FUNC) static_cast<Cpp::TCppFuncAddr_t (*)(Cpp::TCppFunction_t)>(
&Cpp::GetFunctionAddress)},…oads with new macros
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 146. Check the log or trigger a new build to see more.
| #define CPPINTEROP_CPPINTEROPDISPATCH_H | ||
|
|
||
| #include <cstddef> | ||
| #include <cstdint> |
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: included header cstddef is not used directly [misc-include-cleaner]
| #include <cstdint> | |
| #include <cstdint> |
|
|
||
| #include <cstddef> | ||
| #include <cstdint> | ||
| #include <set> |
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: included header cstdint is not used directly [misc-include-cleaner]
| #include <set> | |
| #include <set> |
| #include <cstddef> | ||
| #include <cstdint> | ||
| #include <set> | ||
| #include <string> |
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: included header set is not used directly [misc-include-cleaner]
| #include <string> | |
| #include <string> |
| #include <cstdint> | ||
| #include <set> | ||
| #include <string> | ||
| #include <vector> |
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: included header string is not used directly [misc-include-cleaner]
| #include <vector> | |
| #include <vector> |
| #include <set> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
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: included header vector is not used directly [misc-include-cleaner]
| FOR_EACH_CPP_FUNCTION_OVERLOADED(EXTRACT_NAME_OVERLOADED) | ||
|
|
||
| #define DECLARE_TYPE_SIMPLE(func_name) \ | ||
| using func_name = decltype(&Cpp::func_name); |
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: macro argument should be enclosed in parentheses [bugprone-macro-parentheses]
| using func_name = decltype(&Cpp::func_name); | |
| using (func_name) = decltype(&Cpp::func_name); |
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.
These are good suggestions..
| using func_name = decltype(&Cpp::func_name); | ||
|
|
||
| #define DECLARE_TYPE_OVERLOADED(func_name, signature) \ | ||
| using func_name = signature; |
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: macro argument should be enclosed in parentheses [bugprone-macro-parentheses]
| using func_name = signature; | |
| using (func_name) = signature; |
| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION_SIMPLE(EXTERN_CPP_FUNC_SIMPLE) |
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: variable 'AddIncludePath' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION_SIMPLE(EXTERN_CPP_FUNC_SIMPLE)
^Additional context
include/CppInterOp/CppInterOpDispatch.h:69: expanded from macro 'FOR_EACH_CPP_FUNCTION_SIMPLE'
DO(AddIncludePath) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION_SIMPLE(EXTERN_CPP_FUNC_SIMPLE) |
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: variable 'AddSearchPath' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION_SIMPLE(EXTERN_CPP_FUNC_SIMPLE)
^Additional context
include/CppInterOp/CppInterOpDispatch.h:176: expanded from macro 'FOR_EACH_CPP_FUNCTION_SIMPLE'
DO(AddSearchPath) \
^| return getCppProcAddress ? getCppProcAddress(name) : nullptr; | ||
| } | ||
| namespace CppDispatch { | ||
| FOR_EACH_CPP_FUNCTION_SIMPLE(EXTERN_CPP_FUNC_SIMPLE) |
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: variable 'Allocate' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
FOR_EACH_CPP_FUNCTION_SIMPLE(EXTERN_CPP_FUNC_SIMPLE)
^Additional context
include/CppInterOp/CppInterOpDispatch.h:116: expanded from macro 'FOR_EACH_CPP_FUNCTION_SIMPLE'
DO(Allocate) \
^My requested changes have been dealt with now.
| return; | ||
| } | ||
|
|
||
| getCppProcAddress = reinterpret_cast<void* (*)(const char*)>( |
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.
| getCppProcAddress = reinterpret_cast<void* (*)(const char*)>( | |
| // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) | |
| getCppProcAddress = reinterpret_cast<void* (*)(const char*)>( |
| /// (RTLD_LOCAL) \param[in] customLibPath Optional custom path to | ||
| /// libclangCppInterOp.so \returns true if initialization succeeded, false | ||
| /// otherwise | ||
| inline bool init_functions(const char* customLibPath = 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.
That should be a test case, right?
| # Add the DispatchAPITest only when building shared libraries | ||
| if (BUILD_SHARED_LIBS) | ||
| set_source_files_properties(DispatchAPITest.cpp PROPERTIES COMPILE_DEFINITIONS | ||
| "CPPINTEROP_LIB_DIR=\"${CMAKE_BINARY_DIR}/lib/libclangCppInterOp${CMAKE_SHARED_LIBRARY_SUFFIX}\"" |
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.
Please write a test and try to run that test against installed CppInterOp with the build folder deleted.
|
|
||
| #include "gtest/gtest.h" | ||
|
|
||
| #include <dlfcn.h> |
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.
You probably do not need this header file.
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.
You probably do not need this header file.
Thanks, that's a remnant from earlier
This PR defines the mechanism which enables dispatching CppInterOp's API without linking to it, preventing any LLVM or Clang symbols from being leaked into the client application.
This allows us to run cppyy without linking to CppInterOp motivated by the use case in ROOT. Can be used to deploy CppInterOp in an environment where dynamic linking is not favourable and the only option is dlopen'ing
libClangCppInterOp.sowithRTLD_LOCAL