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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
