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

[Cameron via lldb-commits]

cameron314 added inline comments.

================
Comment at: lldb/trunk/source/Host/common/FileSpec.cpp:242
@@ -221,1 +241,3 @@
+        path.push_back(0);
+        path.pop_back();
     }
----------------
amccarth wrote:
> I recognize that you're just repeating the pattern from above, but ...
> 
> This seems whacky and dangerous.  It appears the attempt is to put a null terminator on the end, but not count it in the length of the vector.  And I guess that we know it's safe here because path is an llvm::SmallVector, so the implementation details are known.  But, ugh.  If `path` were ever changed to std::vector, we'd no longer have assurance that the terminator remains after the pop.
I totally agree. Took me a few minutes before I figured out what it was doing the first time I saw it :-) This is also done in the existing `convertUTF8ToUTF16String` wrapper too.

All the implementations of `std::vector` that I know of will work with this, though as you say it's not guaranteed by the standard.

Looking more closely, I think in this case it was only done for the call to `stat`, which I've removed. I had added it here too in case the caller relied on this null trick, but I don't think it's necessary in either place anymore. I'll remove it.

================
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.
----------------
amccarth wrote:
> I agree with Zach that a dynamic solution here is better.  It's already icky that we have a 32KB stack buffer here.  Turning it into a 64KB +1B stack buffer seem egregious.
Hah, yes. I'll replace this with a vector to start with.


http://reviews.llvm.org/D17107