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