[Lldb-commits] [PATCH] D59854: [Host] Add LibraryLoader abstraction around dlopen/LoadLibrary

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Thanks for the review, Adrian!



================
Comment at: lldb/include/lldb/Host/LibraryLoader.h:17
+public:
+  LibraryLoader(const char *library, bool close = true);
+  ~LibraryLoader();
----------------
amccarth wrote:
> amccarth wrote:
> > I'm curious why anyone would choose `close = false`
> Why `const char *` here when the GetSymbol method takes a StringRef?
I changed it so I could pass a nullptr in the unittest. 


================
Comment at: lldb/include/lldb/Host/LibraryLoader.h:17
+public:
+  LibraryLoader(const char *library, bool close = true);
+  ~LibraryLoader();
----------------
JDevlieghere wrote:
> amccarth wrote:
> > amccarth wrote:
> > > I'm curious why anyone would choose `close = false`
> > Why `const char *` here when the GetSymbol method takes a StringRef?
> I changed it so I could pass a nullptr in the unittest. 
I'm not sure about the semantics of "closing" the library. For the python plugin we call initialize once, so it's fine that the symbol is gone, as long as the library remains in memory. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59854/new/

https://reviews.llvm.org/D59854