[PATCH] D99357: [Support][Windows] Make sure only executables are found by sys::findProgramByName

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

Just some drive-by comments.



================
Comment at: llvm/lib/Support/Windows/Program.inc:65
   SmallVector<StringRef, 12> PathExts;
   PathExts.push_back("");
   PathExts.push_back(".exe"); // FIXME: This must be in %PATHEXT%.
----------------
Adding the empty string to the list of possible extensions for an executable seems like an odd thing to do on Windows (and that's a big part of the reason why SearchPathW is finding directories).  I guess that's attempting to handle the case where Name already has the extension (e.g., "foo.exe").  In that case, I think the empty string should be conditional on whether the Name already has something that looks like an extension.


================
Comment at: llvm/lib/Support/Windows/Program.inc:68
   if (const char *PathExtEnv = std::getenv("PATHEXT"))
     SplitString(PathExtEnv, PathExts, ";");
 
----------------
As the FIXME suggests, adding ".exe" is unnecessary.  Worse it changes the search order.  The PATHEXT environment variable determines not only what extensions are considered executable, but also what order they should be searched.  Usually .COM and .BAT should be found before .EXE.

I would have done:

```C++
if (const char *PathExtEnv = std::getenv("PATHEXT"))
    SplitString(PathExtEnv, PathExts, ";");
else
    PathExts.push_back(".exe");
```



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99357/new/

https://reviews.llvm.org/D99357