Replace std::string pointer by reference and remove unused function DLSym #193
Conversation
SAtacker
commented
Jan 27, 2024
SAtacker
commented
- Add test for DLOpen
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #193 +/- ##
==========================================
+ Coverage 78.83% 78.87% +0.03%
==========================================
Files 8 8
Lines 3048 3048
==========================================
+ Hits 2403 2404 +1
+ Misses 645 644 -1
... and 2 files with indirect coverage changes
|
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
2 similar comments
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -5,6 +5,11 @@ | |||
| #include "llvm/Support/FileSystem.h" | |||
| #include "llvm/Support/Path.h" | |||
|
|
|||
| #include "../../lib/Interpreter/Paths.h" | |||
|
|
|||
| #define XCPP_INTEROP_STRINGIFY(x) #x | |||
There was a problem hiding this comment.
warning: function-like macro 'XCPP_INTEROP_STRINGIFY' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
#define XCPP_INTEROP_STRINGIFY(x) #x
^| #include "../../lib/Interpreter/Paths.h" | ||
|
|
||
| #define XCPP_INTEROP_STRINGIFY(x) #x | ||
| #define CPP_INTEROP_STRINGIFY(x) XCPP_INTEROP_STRINGIFY(x) |
There was a problem hiding this comment.
warning: function-like macro 'CPP_INTEROP_STRINGIFY' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
#define CPP_INTEROP_STRINGIFY(x) XCPP_INTEROP_STRINGIFY(x)
^
SAtacker
force-pushed
the
codecov-inc
branch
3 times, most recently
from
1905eb3
to
1976d0c
Compare
| EXPECT_TRUE(err.find("no such file") != std::string::npos || | ||
| err.find("No such file") != std::string::npos); |
There was a problem hiding this comment.
| EXPECT_TRUE(err.find("no such file") != std::string::npos || | |
| err.find("No such file") != std::string::npos); | |
| EXPECT_FALSE(err.empty()); |
There was a problem hiding this comment.
I feel no such file is more verbose and the exact error we expect.
|
|
||
| TEST(UtilsPlatform, DLTest) { | ||
| std::string err = ""; | ||
| // CPPINTEROP_LIB_TestSharedLib_DIR_PREFIX specified by cmake though target |
There was a problem hiding this comment.
Is that supposed to compute the full path?
There was a problem hiding this comment.
Is that supposed to compute the full path?
That depends what ${CMAKE_BINARY_DIR}/unittests/bin/$<CONFIG>/ gives, we can assume that it gives the full path.
There was a problem hiding this comment.
Why is this necessary? Can't we get the fullpath to the test executable and find back the library?
There was a problem hiding this comment.
I tried to do it but it seemed to vary across arch and platform and configurations.
There was a problem hiding this comment.
If you do not want macros in that we can have a config.h.in file and have cmake configure it.
There was a problem hiding this comment.
I am trying to avoid that but not at the cost of introducing config.h.in. I was wondering if we should not try to look for these files in a few places. Like check if the file exists in these couple of places and pass it to the system.
There was a problem hiding this comment.
I am trying to avoid that but not at the cost of introducing
config.h.in. I was wondering if we should not try to look for these files in a few places. Like check if the file exists in these couple of places and pass it to the system.
We can have a small utility program that uses c++ 17's filesystem and searches for it. Would that work?
There was a problem hiding this comment.
I agree, however, no need to C++ 17 to implement that.
There was a problem hiding this comment.
Alrighty, I shall do it for POSIX and windows separately.
There was a problem hiding this comment.
You can use llvm facilities from llvm::fs and llvm::sys that would avoid this.
SAtacker
force-pushed
the
codecov-inc
branch
from
fb2a112
to
01182ab
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
SAtacker
force-pushed
the
codecov-inc
branch
from
e6651f3
to
9a693a6
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>
SAtacker
force-pushed
the
codecov-inc
branch
from
01fb500
to
bbff1d2
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
unittests/CppInterOp/CMakeLists.txt
Outdated
| @@ -22,7 +22,7 @@ export_executable_symbols(CppInterOpTests) | |||
|
|
|||
| unset(LLVM_LINK_COMPONENTS) | |||
|
|
|||
| add_cppinterop_unittest(DynamicLibraryManagerTests DynamicLibraryManagerTest.cpp) | |||
| add_cppinterop_unittest(DynamicLibraryManagerTests DynamicLibraryManagerTest.cpp ${CMAKE_SOURCE_DIR}/lib/Interpreter/Paths.cpp) | |||
There was a problem hiding this comment.
Why did we add this here? That should be provided by the shared object file.
There was a problem hiding this comment.
It was because it wasn't getting linked on one of the platforms
There was a problem hiding this comment.
Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
| DLErr(Err); | ||
| return Lib; | ||
| void DLClose(void* Lib, std::string& Err) { | ||
| auto dl = llvm::sys::DynamicLibrary(Lib); |
There was a problem hiding this comment.
Can we add a test where we dlopen a library with standard dlopen and not DLOpen and try to close it with DLClose?
