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

[Zachary Turner via lldb-commits]

zturner added a comment.

Also make sure you run `ninja check-lldb` and `ninja check-lldb-unit` with this patch to make sure everything works.


================
Comment at: lldb/trunk/source/Core/ConnectionSharedMemory.cpp:140
@@ +139,3 @@
+    llvm::SmallVector<UTF16, 128> wname;
+    if (llvm::convertUTF8ToUTF16String(name, wname))
+    {
----------------
cameron314 wrote:
> amccarth wrote:
> > 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().
> I didn't want to add/change the string conversion support libraries too much (there's already too many functions with overlapping functionality IMO), but on the whole this would indeed yield much nicer and simpler code. I'll make this change.
It's going to be difficult to make this change.  Because that will require a separate review on the LLVM side with different reviewers.  You can certainly try though.

================
Comment at: lldb/trunk/source/Host/common/File.cpp:357
@@ +356,3 @@
+        {
+            error.SetError(1, lldb::eErrorTypeGeneric);
+            return 0;
----------------
cameron314 wrote:
> amccarth wrote:
> > Why is the error reported here different than many of the other locations?
> About a quarter of the functions report errors through an `Error` (like this one), so I followed suit in this context. Might be better if I set it with a message instead, though -- I'll make that change.
> 
> The rest report errors either through `errno`; by some context-dependent return value (e.g. functions that return false on any failure); or don't check for errors at all.
This is why having the conversion centralized as I suggested earlier would be advantageous.  Everything would always behave the same way.

================
Comment at: lldb/trunk/source/Host/common/FileSpec.cpp:237
@@ +236,3 @@
+    int stat_result;
+#ifdef _WIN32
+    llvm::SmallVector<UTF16, 128> wpath;
----------------
cameron314 wrote:
> amccarth wrote:
> > zturner wrote:
> > > Maybe you can just call `GetStats()` here
> > Instead of another #ifdef here, can't we just re-use the GetFileStats function above?
> Probably, yes, but it requires a FileSpec. I'm not sure what creating a temporary one implicates. Surely there was a reason it wasn't already called before?
This is another reason I argued for moving this to `FileSystem` so that we wouldn't need a `FileSpec` here.  

================
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.
----------------
cameron314 wrote:
> amccarth wrote:
> > MAX_PATH
> `PATH_MAX` :-)
> 
> Like most of the wide Win32 API, this function can return nice long UNC paths.
> 
> `PATH_MAX` should be better named to avoid confusion, though. Maybe `LLDB_MAX_PATH` instead?
Alternatively, you could just call it with 0 and then allocate a `vector` of the appropriate size based on the return value.

================
Comment at: lldb/trunk/source/Host/windows/Windows.cpp:34
@@ +33,3 @@
+{
+    bool utf8ToWide(const char* utf8, wchar_t* buf, size_t bufSize)
+    {
----------------
Do the LLVM functions not work here for some reason?

================
Comment at: lldb/trunk/source/Host/windows/Windows.cpp:210
@@ +209,3 @@
+    wchar_t wpath[PATH_MAX];
+    if (wchar_t* wresult = _wgetcwd(wpath, PATH_MAX))
+    {
----------------
Why can't we just convert to the result of `_wgetcwd` directly to UTF8?

================
Comment at: lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1175
@@ -1166,1 +1174,3 @@
+    fp = _wfopen((const wchar_t *)wpath.data(), (const wchar_t *)wmode.data());
+#else
     fp = fopen(path, mode);
----------------
Here's another example of an #ifdef in common code that we will need to do something about.  Is there a constructor of the `File` class that takes a path and a mode?

================
Comment at: lldb/trunk/source/Target/ProcessLaunchInfo.cpp:457
@@ +456,3 @@
+                    const char *curr_path = nullptr;
+#ifdef _WIN32
+                    const wchar_t *wcurr_path = _wgetenv(L"PATH");
----------------
Anothe here.

================
Comment at: lldb/trunk/tools/driver/Driver.cpp:1277
@@ -1271,1 +1276,3 @@
 
+#if defined(_WIN32)
+    // Convert wide arguments to UTF-8
----------------
This ifdef can probably stay, I don't know if there's a good way around this one.

================
Comment at: lldb/trunk/tools/driver/Driver.cpp:1289
@@ +1288,3 @@
+    // Indicate that all our output is in UTF-8
+    SetConsoleCP(CP_UTF8);
+#endif
----------------
Is this going to affect the state of the console even after quitting LLDB?


Repository:
  rL LLVM

http://reviews.llvm.org/D17107