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