Use reference instead of ptr and remove unused function

Signed-off-by: Shreyas Atre <[email protected]>
compiler-research · Feb 17, 2024 · bbff1d2 · bbff1d2
1 parent e61e7ca
commit bbff1d2
Show file tree

Hide file tree

Showing 5 changed files with 39 additions and 43 deletions.
diff --git a/lib/Interpreter/DynamicLibraryManager.cpp b/lib/Interpreter/DynamicLibraryManager.cpp
@@ -365,7 +365,7 @@ namespace Cpp {
     // TODO: !permanent case
 
     std::string errMsg;
-    DyLibHandle dyLibHandle = platform::DLOpen(canonicalLoadedLib, &errMsg);
+    DyLibHandle dyLibHandle = platform::DLOpen(canonicalLoadedLib, errMsg);
     if (!dyLibHandle) {
       // We emit callback to LibraryLoadingFailed when we get error with error message.
       //TODO: Implement callbacks
@@ -403,7 +403,7 @@ namespace Cpp {
     // TODO: !permanent case
 
     std::string errMsg;
-    platform::DLClose(dyLibHandle, &errMsg);
+    platform::DLClose(dyLibHandle, errMsg);
     if (!errMsg.empty()) {
       LLVM_DEBUG(dbgs() << "cling::DynamicLibraryManager::unloadLibrary(): "
                         << errMsg << '\n');

diff --git a/lib/Interpreter/Paths.cpp b/lib/Interpreter/Paths.cpp
@@ -106,53 +106,38 @@ namespace platform {
     return std::string(Buffer.str());
   }
 
-#if defined(LLVM_ON_UNIX)
-  static void DLErr(std::string* Err) {
-    if (!Err)
-      return;
-    if (const char* DyLibError = ::dlerror())
-      *Err = DyLibError;
+#if defined(_WIN32)
+  void* DLOpen(const std::string& Path, std::string& Err) {
+    auto lib = llvm::sys::DynamicLibrary::getLibrary(Path.c_str(), &Err);
+    return lib.getOSSpecificHandle();
   }
 
-  void* DLOpen(const std::string& Path, std::string* Err /* = nullptr */) {
-    void* Lib = ::dlopen(Path.c_str(), RTLD_LAZY | RTLD_GLOBAL);
-    DLErr(Err);
-    return Lib;
+  void DLClose(void* Lib, std::string& Err) {
+    auto dl = llvm::sys::DynamicLibrary(Lib);
+    llvm::sys::DynamicLibrary::closeLibrary(dl);
+    if (Err.empty()) {
+      Err = std::string();
+    }
+  }
+#elif defined(LLVM_ON_UNIX)
+  static void DLErr(std::string& Err) {
+    if (const char* DyLibError = ::dlerror())
+      Err = std::string(DyLibError);
   }
 
-  void* DLSym(const std::string& Name, std::string* Err /* = nullptr*/) {
-    if (void* Self = ::dlopen(nullptr, RTLD_GLOBAL)) {
-      // get dlopen error if there is one
-      DLErr(Err);
-      void* Sym = ::dlsym(Self, Name.c_str());
-      // overwrite error if dlsym caused one
+  void* DLOpen(const std::string& Path, std::string& Err) {
+    void* Lib = ::dlopen(Path.c_str(), RTLD_LAZY | RTLD_GLOBAL);
+    if (Lib == nullptr) {
       DLErr(Err);
-      // only get dlclose error if dlopen & dlsym haven't emited one
-      DLClose(Self, Err && Err->empty() ? Err : nullptr);
-      return Sym;
+      return nullptr;
     }
-    DLErr(Err);
-    return nullptr;
+    return Lib;
   }
 
-  void DLClose(void* Lib, std::string* Err /* = nullptr*/) {
+  void DLClose(void* Lib, std::string& Err) {
     ::dlclose(Lib);
     DLErr(Err);
   }
-#elif defined(_WIN32)
-
-  void* DLOpen(const std::string& Path, std::string* Err /* = nullptr */) {
-    auto lib = llvm::sys::DynamicLibrary::getLibrary(Path.c_str(), Err);
-    return lib.getOSSpecificHandle();
-  }
-
-  void DLClose(void* Lib, std::string* Err /* = nullptr*/) {
-    auto dl = llvm::sys::DynamicLibrary(Lib);
-    llvm::sys::DynamicLibrary::closeLibrary(dl);
-    if (Err) {
-      *Err = std::string();
-    }
-  }
 #endif
 
 } // namespace platform

diff --git a/lib/Interpreter/Paths.h b/lib/Interpreter/Paths.h
@@ -47,9 +47,7 @@ std::string NormalizePath(const std::string& Path);
 ///
 /// \returns the library handle
 ///
-void* DLOpen(const std::string& Path, std::string* Err = nullptr);
-
-void* DLSym(const std::string& Name, std::string* Err = nullptr);
+void* DLOpen(const std::string& Path, std::string& Err);
 
 ///\brief Close a handle to a shared library.
 ///
@@ -58,7 +56,7 @@ void* DLSym(const std::string& Name, std::string* Err = nullptr);
 ///
 /// \returns the library handle
 ///
-void DLClose(void* Lib, std::string* Err = nullptr);
+void DLClose(void* Lib, std::string& Err);
 } // namespace platform
 
 ///\brief Replace all $TOKENS in a string with environent variable values.

diff --git a/unittests/CppInterOp/CMakeLists.txt b/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)
 target_link_libraries(DynamicLibraryManagerTests
   PRIVATE
   clangCppInterOp

diff --git a/unittests/CppInterOp/DynamicLibraryManagerTest.cpp b/unittests/CppInterOp/DynamicLibraryManagerTest.cpp
@@ -5,6 +5,8 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 
+#include "../../lib/Interpreter/Paths.h"
+
 // This function isn't referenced outside its translation unit, but it
 // can't use the "static" keyword because its address is used for
 // GetMainExecutable (since some platforms don't support taking the
@@ -40,6 +42,17 @@ TEST(DynamicLibraryManagerTest, Sanity) {
       << "Cannot find: '" << PathToTestSharedLib << "' in '" << Dir.str()
       << "'";
 
+  // DLOPEN DLCLOSE Test
+  std::string err = "";
+  auto* dlopen_handle = Cpp::utils::platform::DLOpen(PathToTestSharedLib, err);
+  EXPECT_TRUE(dlopen_handle) << "Error occurred: " << err << "\n";
+  Cpp::utils::platform::DLClose(dlopen_handle, err);
+  EXPECT_TRUE(err.empty()) << "Error occurred: " << err << "\n";
+  Cpp::utils::platform::DLOpen("missing", err);
+  EXPECT_TRUE(err.find("no such file") != std::string::npos ||
+              err.find("No such file") != std::string::npos);
+  // DLOPEN DLCLOSE Test end
+
   EXPECT_TRUE(Cpp::LoadLibrary(PathToTestSharedLib.c_str()));
   // Force ExecutionEngine to be created.
   Cpp::Process("");