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

[Pavel Labath via Phabricator via lldb-commits]

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 {
+public:
----------------
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;
+#else
----------------
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.


https://reviews.llvm.org/D30789