Replace std::string pointer by reference and remove unused function DLSym #193

SAtacker · 2024-01-27T23:08:54Z

Add test for DLOpen

github-actions

clang-tidy made some suggestions

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

codecov · 2024-01-27T23:13:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.87%. Comparing base (e61e7ca) to head (2315dea).
Report is 11 commits behind head on main.

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

Files	Coverage Δ
lib/Interpreter/DynamicLibraryManager.cpp	`75.12% <100.00%> (ø)`
lib/Interpreter/Paths.cpp	`37.75% <100.00%> (+2.34%)`	⬆️

... and 2 files with indirect coverage changes

Files	Coverage Δ
lib/Interpreter/DynamicLibraryManager.cpp	`75.12% <100.00%> (ø)`
lib/Interpreter/Paths.cpp	`37.75% <100.00%> (+2.34%)`	⬆️

... and 2 files with indirect coverage changes

github-actions · 2024-01-27T23:27:14Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-01-27T23:56:11Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-01-28T00:04:28Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-01-28T00:19:47Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-01-28T00:41:05Z

clang-tidy review says "All clean, LGTM! 👍"

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

github-actions · 2024-02-03T18:25:14Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-02-03T18:46:53Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-02-04T01:37:15Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions

clang-tidy made some suggestions

github-actions · 2024-02-04T05:54:28Z

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

@@ -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


warning: function-like macro 'XCPP_INTEROP_STRINGIFY' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]

#define XCPP_INTEROP_STRINGIFY(x) #x ^

github-actions · 2024-02-04T05:54:29Z

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

+#include "../../lib/Interpreter/Paths.h"
+
+#define XCPP_INTEROP_STRINGIFY(x) #x
+#define CPP_INTEROP_STRINGIFY(x) XCPP_INTEROP_STRINGIFY(x)


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) ^

vgvassilev · 2024-02-04T08:15:35Z

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

+  EXPECT_TRUE(err.find("no such file") != std::string::npos ||
+              err.find("No such file") != std::string::npos);


Suggested change

EXPECT_TRUE(err.find("no such file") != std::string::npos ||

err.find("No such file") != std::string::npos);

EXPECT_FALSE(err.empty());

I feel no such file is more verbose and the exact error we expect.

vgvassilev · 2024-02-04T08:17:10Z

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

+
+TEST(UtilsPlatform, DLTest) {
+  std::string err = "";
+  // CPPINTEROP_LIB_TestSharedLib_DIR_PREFIX specified by cmake though target


Is that supposed to compute the full path?

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.

Why is this necessary? Can't we get the fullpath to the test executable and find back the library?

I tried to do it but it seemed to vary across arch and platform and configurations.

If you do not want macros in that we can have a config.h.in file and have cmake configure it.

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.

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?

I agree, however, no need to C++ 17 to implement that.

Alrighty, I shall do it for POSIX and windows separately.

You can use llvm facilities from llvm::fs and llvm::sys that would avoid this.

lib/Interpreter/Paths.cpp

github-actions · 2024-02-16T22:21:41Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-02-16T22:40:07Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions

clang-tidy made some suggestions

unittests/CppInterOp/DynamicLibraryManagerTest.cpp

github-actions · 2024-02-17T03:56:48Z

clang-tidy review says "All clean, LGTM! 👍"

Signed-off-by: Shreyas Atre <[email protected]>

github-actions · 2024-02-17T04:18:41Z

clang-tidy review says "All clean, LGTM! 👍"

vgvassilev · 2024-02-17T08:26:49Z

unittests/CppInterOp/CMakeLists.txt

@@ -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)


Why did we add this here? That should be provided by the shared object file.

It was because it wasn't getting linked on one of the platforms

PTAL https://github.com/compiler-research/CppInterOp/actions/runs/7944478504/job/21690038529?pr=193#step:20:172 @vgvassilev

Signed-off-by: Shreyas Atre <[email protected]>

github-actions · 2024-02-17T21:54:15Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2024-03-11T03:24:55Z

clang-tidy review says "All clean, LGTM! 👍"

Signed-off-by: Shreyas Atre <[email protected]>

github-actions · 2024-03-11T03:29:29Z

clang-tidy review says "All clean, LGTM! 👍"

vgvassilev · 2024-03-18T14:53:49Z

lib/Interpreter/Paths.cpp

-    DLErr(Err);
-    return Lib;
+  void DLClose(void* Lib, std::string& Err) {
+    auto dl = llvm::sys::DynamicLibrary(Lib);


Can we add a test where we dlopen a library with standard dlopen and not DLOpen and try to close it with DLClose?

github-actions bot reviewed Jan 27, 2024

View reviewed changes

unittests/CppInterOp/DynamicLibraryManagerTest.cpp Outdated Show resolved Hide resolved

SAtacker force-pushed the codecov-inc branch from f05eb30 to 6d3a759 Compare January 27, 2024 23:23

SAtacker force-pushed the codecov-inc branch from f34b610 to d107d24 Compare January 28, 2024 00:00

SAtacker force-pushed the codecov-inc branch from be4ba59 to bd8d0e9 Compare January 28, 2024 00:37

vgvassilev reviewed Jan 28, 2024

View reviewed changes

unittests/CppInterOp/DynamicLibraryManagerTest.cpp Outdated Show resolved Hide resolved

SAtacker force-pushed the codecov-inc branch from 39b920e to 0c9491d Compare February 3, 2024 18:21

github-actions bot reviewed Feb 4, 2024

View reviewed changes

SAtacker force-pushed the codecov-inc branch 3 times, most recently from 1905eb3 to 1976d0c Compare February 4, 2024 06:08

vgvassilev reviewed Feb 4, 2024

View reviewed changes

lib/Interpreter/Paths.cpp Show resolved Hide resolved

SAtacker force-pushed the codecov-inc branch from fb2a112 to 01182ab Compare February 16, 2024 22:17

github-actions bot reviewed Feb 16, 2024

View reviewed changes

unittests/CppInterOp/DynamicLibraryManagerTest.cpp Outdated Show resolved Hide resolved

SAtacker force-pushed the codecov-inc branch from e6651f3 to 9a693a6 Compare February 16, 2024 22:53

Use reference instead of ptr and remove unused function

bbff1d2

Signed-off-by: Shreyas Atre <[email protected]>

SAtacker force-pushed the codecov-inc branch from 01fb500 to bbff1d2 Compare February 17, 2024 04:14

vgvassilev reviewed Feb 17, 2024

View reviewed changes

Test without having path.cpp into sources

a242ddb

Signed-off-by: Shreyas Atre <[email protected]>

Compiler Dynamic Library Manager test only if Clang-repl is used

2315dea

Signed-off-by: Shreyas Atre <[email protected]>

SAtacker force-pushed the codecov-inc branch from 9010f4a to 2315dea Compare March 11, 2024 03:25

vgvassilev reviewed Mar 18, 2024

View reviewed changes

		EXPECT_TRUE(err.find("no such file") != std::string::npos \|\|
		err.find("No such file") != std::string::npos);

	EXPECT_TRUE(err.find("no such file") != std::string::npos \|\|
	err.find("No such file") != std::string::npos);
	EXPECT_FALSE(err.empty());

Replace std::string pointer by reference and remove unused function DLSym #193

Are you sure you want to change the base?

Replace std::string pointer by reference and remove unused function DLSym #193

Conversation

SAtacker commented Jan 27, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 27, 2024 • edited Loading

Codecov Report

github-actions bot commented Jan 27, 2024

github-actions bot commented Jan 27, 2024

github-actions bot commented Jan 28, 2024

github-actions bot commented Jan 28, 2024

github-actions bot commented Jan 28, 2024

github-actions bot commented Feb 3, 2024

github-actions bot commented Feb 3, 2024

github-actions bot commented Feb 4, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Feb 4, 2024

Choose a reason for hiding this comment

github-actions bot Feb 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 16, 2024

github-actions bot commented Feb 16, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 17, 2024

github-actions bot commented Feb 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 17, 2024

github-actions bot commented Mar 11, 2024

github-actions bot commented Mar 11, 2024

Choose a reason for hiding this comment

codecov bot commented Jan 27, 2024 •

edited

Loading