[Lldb-commits] [PATCH] D30789: Make file and directory name completion work properly on Windows.

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Fri Mar 10 01:59:30 PST 2017

labath added a comment.

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.

Comment at: lldb/include/lldb/Utility/TildeExpressionResolver.h:18
+namespace lldb_private {
+struct TildeExpressionResolver {
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.

Comment at: lldb/source/Commands/CommandCompletions.cpp:112
+  if (!Resolver)
+    Resolver = &SR;
amccarth wrote:
> 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.
I agree with Adrian.
What do you think about an API like:
`DiskFilesOrDirectories(..., const TildeExpressionResolver &Resolver = StandardTildeExpressionResolver())` ?

Comment at: lldb/source/Commands/CommandCompletions.cpp:116
+  llvm::SmallString<PATH_MAX> CompletionBuffer;
+  llvm::SmallString<PATH_MAX> Storage;
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?

Comment at: lldb/source/Commands/CommandCompletions.cpp:167
-  if (remainder[0] == '\0' || strstr(name, remainder) == name) {
-    if (strlen(name) + parameters->baselen >= PATH_MAX)
-      return FileSpec::eEnumerateDirectoryResultNext;
+    // We want to keep the form the user typed, so we special case this to
+    // search
This comment could use some reformatting.

Comment at: lldb/source/Utility/TildeExpressionResolver.cpp:44
+#if defined(LLVM_ON_WIN32)
+  return false;
return 0;

Comment at: lldb/source/Utility/TildeExpressionResolver.cpp:49
+  struct passwd *user_entry;
+  Expr = Expr.drop_front();
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.


More information about the lldb-commits mailing list