amccarth added a comment.
The Windows code looks correct to me, but I've added a couple inline
questions/comments.
I can patch it in and test it for you tomorrow morning if nobody else has done
it by then. I think zturner's still on vacation for another day or two.
================
Comment at: lldb/include/lldb/Host/LibraryLoader.h:17
+public:
+ LibraryLoader(const char *library, bool close = true);
+ ~LibraryLoader();
----------------
I'm curious why anyone would choose `close = false`
================
Comment at: lldb/include/lldb/Host/LibraryLoader.h:17
+public:
+ LibraryLoader(const char *library, bool close = true);
+ ~LibraryLoader();
----------------
amccarth wrote:
> I'm curious why anyone would choose `close = false`
Why `const char *` here when the GetSymbol method takes a StringRef?
================
Comment at: lldb/include/lldb/Host/LibraryLoader.h:20
+
+ operator bool() { return m_handle != nullptr; }
+
----------------
Probably should be a const method.
This will work on Windows, since we know this handle is from LoadLibrary. Some
other Windows handles can signal an error with INVALID_HANDLE_VALUE rather than
a null pointer, but LoadLibrary doesn't. If you wanted to be extra careful,
you could delegate the validity check to the concrete type, but that doesn't
seem necessary and it looks like we could change it later without affecting the
interface.
================
Comment at: lldb/source/Host/windows/LibraryLoader.cpp:17
+ : m_handle(nullptr), m_close(close) {
+ m_handle = reinterpret_cast<void *>(LoadLibrary(library));
+}
----------------
Since you're passing a char string to LoadLibrary, you might want to explicitly
call LoadLibraryA (note the -A suffix).
I know that we generally don't support Unicode well in filenames throughout the
LLVM system. If you know the encoded string is UTF-8, you could convert it to
UTF-16 and call LoadLibraryW. If you leave it to LoadLibraryA, it'll use the
user's current default code page, which almost certainly is not UTF-8.
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59854/new/
https://reviews.llvm.org/D59854
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
