[PATCH] [Support][Program] Add findProgramByName(Name, OptionalPaths)

Michael Spencer bigcheesegs at gmail.com
Mon Nov 3 12:50:46 PST 2014


================
Comment at: include/llvm/Support/Program.h:70
@@ +69,3 @@
+      StringRef Name,
+      Optional<ArrayRef<StringRef>> Paths = Optional<ArrayRef<StringRef>>());
+
----------------
rafael wrote:
> Why do we need the Optional? Can't we just use an empty Paths as a special value that means "read $PATH"?
K.

================
Comment at: lib/Support/Unix/Program.inc:107
@@ +106,3 @@
+  // Check some degenerate cases
+  if (Name.empty()) // no program
+    return std::errc::no_such_file_or_directory;
----------------
rafael wrote:
> Can't we just say that name should not be empty and assert? It looks like a misuse of the API, not an error.
I was duplicating the behavior of FindProgramByName. I'm not sure if anything relies on the behavior.

================
Comment at: lib/Support/Unix/Program.inc:111
@@ +110,3 @@
+  // the behavior of sh(1) and friends.
+  if (Name.find('/') != std::string::npos)
+    return std::string(Name);
----------------
shankarke wrote:
> StringRef::npos ?
K.

================
Comment at: lib/Support/Unix/Program.inc:114
@@ +113,3 @@
+
+  SmallVector<StringRef, 16> SearchPaths;
+  if (Paths)
----------------
rafael wrote:
> Why do you need the extra storage for all cases? Can't this be
> 
> if (using $PATH) {
>   build the array
>   return  recurse passing the array
> }
> 
> use the arrayref argument directly
K.

http://reviews.llvm.org/D6067






More information about the llvm-commits mailing list