Page MenuHomePhabricator

Make file and directory name completion work properly on Windows.
ClosedPublic

Authored by zturner on Mar 9 2017, 1:18 PM.

Details

Summary

There were a couple of problems with this function on Windows. Different separators and differences in how tilde expressions are resolved for starters, but in addition there was no clear indication of what the function's inputs or outputs were supposed to be, and there were no tests to demonstrate its use.

To more easily paper over the differences between Windows paths, non-Windows paths, and tilde expressions, I've ported this function to use LLVM-based directory iteration (in fact, I would like to eliminate all of LLDB's directory iteration code entirely since LLVM's is cleaner / more efficient (i.e. it invokes fewer stat calls)). and llvm's portable path manipulation library.

Since file and directory completion assumes you are referring to files and directories on your local machine, it's safe to assume the path syntax properties of the host in doing so, so LLVM's APIs are perfect for this.

I've also added a fairly robust set of unit tests. Since you can't really predict what users will be on your machine, or what their home directories will be, I added an interface called TildeExpressionResolver, and in the unit test I've mocked up a fake implementation that acts like a unix password database. This allows us to configure some fake users and home directories in the test, so we can exercise all of those hard-to-test codepaths that normally otherwise depend on the host.

After this patch, file and directory completion works on Windows, and the same tests pass without my patch which suggests that I have not broken anything.

Note that I think there was a bug in the original implementation regarding symlinks which I've tried to fix. Previously, if you tried to complete /foo/bar/baz, and it finds /foo/bar/bazel which is a symlink, then it treats this as a directory as long as /foo/bar is a directory, which doesn't make any sense.

To fix this, if /foo/bar/bazel is a symlink, I stat it again with symlink-following on (which the previous code did not do), and check if the result is a directory.

There's no good way to test this, however.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.Mar 9 2017, 1:18 PM
jingham edited edge metadata.Mar 9 2017, 1:41 PM

What is the motivation behind of the "2" versions of the functions you added?

What is the motivation behind of the "2" versions of the functions you added?

Ahh yea. I can call them something else if you have a better name. But basically in a unit test I don't have an instance of a CommandInterpreter object, and creating one is non-trivial (it requires a Debugger instance, which requires calling Debugger::CreateInstance(), which depends on a lot of other system initialization having been done.

But none of the methods I was trying to test use the CommandInterpreter anyway, so I just needed some kind of interface into the completion that didn't require me to pass one in.

amccarth added inline comments.
lldb/source/Commands/CommandCompletions.cpp
108 ↗(On Diff #91212)

I know you're not inventing this API, but since you're changing it, can you do something about the bool and bool & parameters? It makes reading the call sites difficult without referring back here to see what those mean.

112 ↗(On Diff #91212)

This leaves the caller with no way to say the don't want a tilde resolver. I didn't expect that from reading the API. Perhaps a comment on the API could clarify that you're going to get this default behavior if you specify nullptr.

labath edited edge metadata.Mar 10 2017, 1:59 AM

Seems very reasonable. A couple of details I noticed are in the inline comments.

As for the name, I'd suggest just dropping the 2 and letting overload resolution handle that. If we don't need the interpreter argument, then maybe we should aim for removing that overload in the future.

lldb/include/lldb/Utility/TildeExpressionResolver.h
18 ↗(On Diff #91212)

Is there a reason why one these is a struct and the other a class?

btw, if you move the destructor definition into the .cpp file, it will also serve as a key method.

lldb/source/Commands/CommandCompletions.cpp
112 ↗(On Diff #91212)

I agree with Adrian.
What do you think about an API like:
DiskFilesOrDirectories(..., const TildeExpressionResolver &Resolver = StandardTildeExpressionResolver()) ?

116 ↗(On Diff #91212)

in PosixApi.h you define PATH_MAX as 32768, which sees a bit too much for a stack object. As this doesn't actually impact correctness, should we use a more down-to-earth value here?

167 ↗(On Diff #91212)

This comment could use some reformatting.

lldb/source/Utility/TildeExpressionResolver.cpp
44 ↗(On Diff #91212)

return 0;

49 ↗(On Diff #91212)

Is this function allowed to be called with an empty Expr? The assert above seems to indicate that is the case, but then this will be undefined.

zturner updated this revision to Diff 91361.Mar 10 2017, 9:16 AM

Addressed suggestions from labath@

zturner added inline comments.Mar 10 2017, 9:18 AM
lldb/source/Commands/CommandCompletions.cpp
112 ↗(On Diff #91212)

I don't actually think there *should* be a way to specify no resolver. That sounds like YAGNI. The only real use cases are "Do it for real" or "do it with a mock implementation". I've updated this to take a reference, which should indicate that it is required, and now the version that delegates to this function allocates a real instance in its own stack frame and passes it in. Hopefully this makes things more clear.

labath accepted this revision.Mar 10 2017, 11:16 AM

lgtm

This revision is now accepted and ready to land.Mar 10 2017, 11:16 AM
This revision was automatically updated to reflect the committed changes.