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

[Zachary Turner via lldb-commits]

zturner added inline comments.

================
Comment at: lldb/trunk/source/Commands/CommandCompletions.cpp:171
@@ -168,1 +170,3 @@
         {
+#ifdef _WIN32
+            llvm::SmallVector<UTF16, 128> wpartial_name_copy;
----------------
cameron314 wrote:
> zturner wrote:
> > I'm not fond of this approach of sprinkling #ifdefs around in every location where we call into a file system API.  We have `lldb/Host/FileSystem`, probably some of these methods should be moved over there, and we should provide a windows implementation and a non-windows implementation.  This way in places like this, you simply write:
> > 
> >     isa_directory = FileSystem::GetPermissions(partial_name_copy) & FileSystem::eDirectory);
> > 
> > and all this ugliness is hidden.
> I agree. Again, my goal was not to refactor (I don't know this codebase very well at all and can't test it in much depth), but to fix existing code. It happens that `stat` is particularly awkward on Win32, so yes, it should probably be wrapped instead (or better yet replaced with `GetFileAttributes()`). But this is outside the scope of the patch.
I know your intent was to just fix existing code, but when those fixes make the code harder to maintain or do things the wrong way, then I think it's worth asking whether the scope should expand.  Introducing ifdefs in common code is not a direction we want to move in unless it's absolutely necessary, so somehow we have to address that before this can go in, even if it means expanding the scope of the patch slightly.


Repository:
  rL LLVM

http://reviews.llvm.org/D17107