[Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Adrian McCarthy via lldb-commits]

amccarth added a subscriber: amccarth.
amccarth added a comment.

I like that you're doing this, but ...

There's a ton of duplication here.  I'd really like to see the conversions centralized with consistent error handling.  Cluttering up already-too-long functions with more #ifdefs and a half dozen lines of boilerplate (that's working at a much lower level of abstraction than the surrounding code) doesn't seem like moving in the right direction.


================
Comment at: lldb/trunk/source/Core/ConnectionSharedMemory.cpp:140
@@ +139,3 @@
+    llvm::SmallVector<UTF16, 128> wname;
+    if (llvm::convertUTF8ToUTF16String(name, wname))
+    {
----------------
It's too bad there isn't an overload that gives the wide string as a std::wstring.  Then we wouldn't need all the ugly C-style casts of wname.data().

================
Comment at: lldb/trunk/source/Host/common/File.cpp:330
@@ +329,3 @@
+        }
+        m_descriptor = ::_wopen((const wchar_t *)wpath.data(), oflag, mode);
+#else
----------------
_wopen is officially deprecated.  How about _wsopen_s?

================
Comment at: lldb/trunk/source/Host/common/File.cpp:357
@@ +356,3 @@
+        {
+            error.SetError(1, lldb::eErrorTypeGeneric);
+            return 0;
----------------
Why is the error reported here different than many of the other locations?

================
Comment at: lldb/trunk/source/Host/common/FileSpec.cpp:237
@@ +236,3 @@
+    int stat_result;
+#ifdef _WIN32
+    llvm::SmallVector<UTF16, 128> wpath;
----------------
Instead of another #ifdef here, can't we just re-use the GetFileStats function above?

================
Comment at: lldb/trunk/source/Host/common/FileSpec.cpp:1141
@@ -1103,1 +1140,3 @@
+
         WIN32_FIND_DATA ffd;
+        HANDLE hFind = FindFirstFileW((LPCWSTR)wszDir.data(), &ffd);
----------------
WIN32_FIND_DATATW to be consistent with calling the wide versions of the functions explicitly.

================
Comment at: lldb/trunk/source/Host/common/FileSpec.cpp:1179
@@ +1178,3 @@
+
+            char child_path[PATH_MAX];
+            const int child_path_len = ::snprintf (child_path, sizeof(child_path), "%s\\%s", dir_path, fileName.c_str());
----------------
MAX_PATH in Windows.  (And someday, we should get rid of these fixed buffers so we can use UNC paths.)

================
Comment at: lldb/trunk/source/Host/windows/FileSystem.cpp:231
@@ -191,3 +230,3 @@
 
-    char buf[PATH_MAX];
+    wchar_t buf[PATH_MAX + 1];
     // Subtract 1 from the path length since this function does not add a null terminator.
----------------
MAX_PATH

================
Comment at: lldb/trunk/source/Host/windows/Host.cpp:73
@@ -70,3 +72,3 @@
         // Get the process image path.  MAX_PATH isn't long enough, paths can actually be up to 32KB.
-        std::vector<char> buffer(32768);
+        std::vector<UTF16> buffer(PATH_MAX);
         DWORD dwSize = buffer.size();
----------------
The comment above says why you can't use MAX_PATH/PATH_MAX.  If that's no longer relevant, please update the comment.

================
Comment at: lldb/trunk/source/Host/windows/Host.cpp:160
@@ -158,3 +159,3 @@
 
-    std::vector<char> buffer(MAX_PATH);
+    std::vector<wchar_t> buffer(PATH_MAX);
     DWORD chars_copied = 0;
----------------
MAX_PATH on Windows.


Repository:
  rL LLVM

http://reviews.llvm.org/D17107