[PATCH] D51941: [Support] Avoid calling CommandLineToArgvW from shell32.dll

[Zachary Turner via Phabricator via llvm-commits]

zturner added a comment.

When you commit this, make sure to include in the commit message how much speedup you're seeing as a result of this.



================
Comment at: llvm/lib/Support/Windows/Process.inc:147
 /// doesn't have wildcards or doesn't match any files.
-static std::error_code WildcardExpand(const wchar_t *Arg,
+static std::error_code WildcardExpand(const char *Arg,
                                       SmallVectorImpl<const char *> &Args,
----------------
Can you make the first arg a `StringRef`?


================
Comment at: llvm/lib/Support/Windows/Process.inc:150
+                                      StringSaver &Saver) {
+  std::error_code ec;
 
----------------
`EC`?


================
Comment at: llvm/lib/Support/Windows/Process.inc:155
+  // option.
+  if (!strpbrk(Arg, "*?") || strcmp(Arg, "/?") == 0 || strcmp(Arg, "-?") == 0) {
+    Args.push_back(Arg);
----------------
Then you can write this as `if (Arg.find_first_of("*?") == StringRef::npos || Arg == "/?" || Arg == "-?")`

(one could argue we should have a function called `StringRef::contains_any_of(StringRef Chars)).`  In any case, even though it's a bit longer I find the additional clarity from meaningful function names better than cryptic names like `strpbrk` which I have to look up on cppreference every time I see it.


================
Comment at: llvm/lib/Support/Windows/Process.inc:179
+  // Extract any directory part of the argument.
+  SmallVector<char, MAX_PATH> Dir;
+  Dir.append(ArgStr.begin(), ArgStr.end());
----------------
This can just be a `SmallString<MAX_PATH>`


================
Comment at: llvm/lib/Support/Windows/Process.inc:185
   do {
     SmallVector<char, MAX_PATH> FileName;
     ec = windows::UTF16ToUTF8(FileData.cFileName, wcslen(FileData.cFileName),
----------------
I think this can just be a `SmallString<MAX_PATH>`


================
Comment at: llvm/lib/Support/Windows/Process.inc:192
     // Append FileName to Dir, and remove it afterwards.
     llvm::sys::path::append(Dir, StringRef(FileName.data(), FileName.size()));
+    Args.push_back(Saver.save(Dir).data());
----------------
And you can just write `append(Dir, FileName);`


https://reviews.llvm.org/D51941